-
Notifications
You must be signed in to change notification settings - Fork 132
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
base: conformance-tests-into-docs
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
...as this PR, #1426, does that
There was a problem hiding this 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/Basic-Tests.md
Outdated
| 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. | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
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). |
/netlify |
@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? |
Co-authored-by: kemerava <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/netlify |
closing to reopen (in the hopes that wakes netlify up) |
@kriswest I believe Netlify should have now rebuilt the preview. Can you confirm? |
Confirmed, thanks @TheJuanAndOnly99 |
resolves #1415
supersedes #1271
Adds conformance tests for FDC3 2.2.
Incorporates changes from/follows on from #1417.
Review deep links: