Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conformance additions for 2.2 (second attempt) #1426

Open
wants to merge 17 commits into
base: conformance-tests-into-docs
Choose a base branch
from

Conversation

kriswest
Copy link
Contributor

@kriswest kriswest commented Nov 11, 2024

@kriswest kriswest requested a review from a team as a code owner November 11, 2024 18:06
Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/67658bda6d04b0b3703a2520
😎 Deploy Preview https://deploy-preview-1426--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kriswest kriswest changed the base branch from main to fdc3-for-web November 11, 2024 18:14
@kriswest kriswest changed the base branch from fdc3-for-web to main November 13, 2024 17:00
@kriswest kriswest changed the base branch from main to conformance-tests-into-docs November 14, 2024 12:31
@kriswest kriswest requested a review from a team December 4, 2024 12:19
Copy link
Contributor

@kemerava kemerava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kriswest!
Another general comment/question: considering that there is also .Net (and any other future language bindings), and as we are introducing it effectively with version 2.2, should this be somehow called out at the very least in the docs here?

docs/api/conformance/Intents-Tests.md Outdated Show resolved Hide resolved
docs/api/conformance/Basic-Tests.md Outdated Show resolved Hide resolved
| App | Step | Description |
|-----|-----------------|----------------------------------------------------------|
| A | `getAgent` | A calls `getAgent` and waits for the promise to resolve to a `DesktopAgent` instance. |
| A | `getInfo` | A can call the `getInfo()` method on the `DesktopAgent` instance to get the `ImplementationMetadata` object. <br /> Check that fdc3Version is set to 2.2. <br />Check that provider and providerVersion are populated. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are specifically requesting the provider and its version and in the basic test below (BasicGI1) we are just calling out "provider details", should we either update the former or the latter just to be either more specific or more vague? (I would prefer to be more specific, but not sure if the test actually checks for the version in the latter case and if that is intentional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also similarly just like a general comment, a lot of these rest definitions are written in different styles across all of them, like here it is "A can" in other files it was "should", and sometimes it is just described in present tense. That is a scope creep for this PR but maybe others agree that it should be more standardized, we can create a followup issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the definitions are less than consistent and do need a clean-up. @robmoffat is keen to re-write all these cases and their implementation in gherkin syntax (although it's not clear that he will have time to do so next year - so that may be on the maintainers to do), which would be a good time to standardize them.

I would happily support and issue to fix the language (without changing meaning) and maybe able to collaborate on implementing that fix. It is out of scope for this PR however - but could be a good thing to do within 2.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in the basic tests we say all of 'You can', 'An application can', 'The application should' with no variance in meaning - that is indeed horribly inconsistent!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kemerava I've tried to make the language in the basic tests more consistent (haven't touched the others). I've also made BasicGI1 more specific about the FDC3 version (it should match the version being tested against for conformance).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(those changes were made in #1417

docs/api/conformance/Overview.md Outdated Show resolved Hide resolved
@kriswest
Copy link
Contributor Author

Thanks @kriswest! Another general comment/question: considering that there is also .Net (and any other future language bindings), and as we are introducing it effectively with version 2.2, should this be somehow called out at the very least in the docs here?

There is copy on the compliance page (https://deploy-preview-1426--fdc3.netlify.app/docs/next/fdc3-compliance#conformance-testing) saying that these tests are implemented for Javascript/Typescript by the conformance framework - but not explicitly noted that they have not been implemented for .NET. The test definitions are separate from their implementation and I think (all?) will still be valid for .NET without changes (although a review would be needed to be sure).

@kriswest
Copy link
Contributor Author

/netlify

@kriswest
Copy link
Contributor Author

@robmoffat or @TheJuanAndOnly99 this PR's preview is out of date. I can't tell why netlify is not re-running it after further commits to files in /docs (which is moving to /website/docs/ in a later PR). Could you take a look please?

@kriswest kriswest requested a review from kemerava December 10, 2024 18:31
Copy link
Contributor

@kemerava kemerava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@robmoffat
Copy link
Member

/netlify

@kriswest
Copy link
Contributor Author

closing to reopen (in the hopes that wakes netlify up)

@kriswest kriswest closed this Dec 16, 2024
@kriswest kriswest reopened this Dec 16, 2024
@TheJuanAndOnly99
Copy link
Member

@kriswest I believe Netlify should have now rebuilt the preview. Can you confirm?

@kriswest
Copy link
Contributor Author

@kriswest I believe Netlify should have now rebuilt the preview. Can you confirm?

Confirmed, thanks @TheJuanAndOnly99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update conformance test definitions for FDC3 2.2
4 participants