-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add conformance tests into docs #1417
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@openfin-johans this might be a good docs PR for you to take a quick look at Johan. |
what's your thinking here, @kriswest? Why do we even want to do this? |
Do you mean why would we want to put the conformance test definitions into the docs? Over the last 6 months or so we've answered numerous clarifying questions to 3 teams implementing Desktop Agents. Of those at least 2, if not all 3 were referred to the conformance tests to clarify the intended behaviour in particular areas - they did not discover the tests on their own (because they are not in the documentation), but did find them useful. Two of those teams have since applied for and achieved conformance, by passing tests that implement the test definitions. Hence, the tests are useful additional detail; by adding them to the docs we make them easier to find and to scrutinize/comment on/suggest extensions and corrections. They are more formally part of the standard. We also want to start having PRs that alter the standard also address necessary changes or additions to the conformance test in the same PR - something we agreed to add to the checklist in the proposed PR template. Having the tests visible in the docs will make that easier. Finally, the tests should be better tied to specific standard versions than they are. This was a good opportunity to break from the current format which blends 1.2 and 2.0 tests (which gets preserved in the 2.0 and 2.1 versions) and to create a cleaner version for 2.2 onwards. I think that will help with your proposed rewrite of the implementation (and presumably definition of cases) in gherkin syntax for use with cucumber. |
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.
LGTM
@novavi I dealt with your review comments on this, could you approve it for merge? |
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.
Looks good to me - only founds some minor reference issues and typos.
@openfin-johans @kemerava Comments all dealt with, could you re-approve so we can merge please! |
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.
Ship it captain!
- `BasicGI1`: An `ImplementationMetadata` object can be retrieved, to find out the version of FDC3 in in use along with details of rhe provider, by calling: | ||
- `await fdc3.getInfo()` | ||
- `BasicGI1`: An `ImplementationMetadata` object can be retrieved, to find out the version of FDC3 that is in use along with details of the provider, by calling: | ||
- `await fdc3.getInfo()`.w The FDC3 version should match the API version being tested 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.
@kriswest Accidental leading character. Change ".w The FDC3 version" to ". The FDC3 version".
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.
done on #1426
| B | 2. Retrieve `Channel` | Retrieve a `Channel` object representing the same 'App' channel A did (`test-channel`)| | ||
| B | 3. Broadcast | B broadcasts both the instrument context and a contact context, using: <br /> `testChannel.broadcast(<fdc3.instrument>)` <br /> `testChannel.broadcast(<fdc3.contact>)` | | ||
| A | 4. Add Context Listener | A adds a context listener to the channel _after_ B has completed all its broadcasts, via: <br />`await testChannel.addContextListener("fdc3.instrument", handler)` <br /> Ensure that A does NOT receive any context via these listeners (past context is only retrieved via a `getCurrentContext()` call on App channels). | | ||
| A | 5. Retrieve Current Context | A is able to retrieve the most recent context of each context type from the `Channel` via: <br />`const instrument = await testChannel.getCurrentContext('fdc3.instrument')`<br />`const contact = await testChannel.getCurrentContext('fdc3.contact')`<br />Ensure that both contexts retrieved by A are identical to those sent by B| |
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.
@kriswest Change both getCurrentContext(..)
references from single quotes to double quotes.
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.
done on #1426
| F | Find Intent & Raise Intent with PrivateChannel result | `sharedTestingIntent2(testContextY) => channel<testContextZ>` * | addIntentListener() for given intents | | ||
| G | Find Intent tests (never started) | `sharedTestingIntent2(testContextY)` | addIntentListener() for given intents | | ||
| H | Raise Intent (bad config/behavior) | `sharedTestingIntent2(testContextY) => testContextZ` | - no action | | ||
| I | Raise Intent (bad config/behavior) | `sharedTestingIntent2(testContextY) => testContextZ` | addIntentListener(‘MadeUpIntent’, handler) | |
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.
@kriswest Double quotes - change ‘MadeUpIntent’ to "MadeUpIntent".
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.
done on #1426
|
||
| App | Step | Details | | ||
|------|---------------------------------------|---------------------------------------------------------------------------------------------------| | ||
| Test | 1. Raise intent | Test raises an intent with `fdc3.raiseIntent(‘"kTestingIntent",testContextX,{appId:"<K's appId>"})`<br />starts app K. | |
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.
@kriswest Unwanted smart single quote after open bracket - change (‘"kTestingIntent" to ("kTestingIntent"
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.
done on #1426
resolves #1077
Adds the FDC3 conformance tests into the main documentation and docs for the 2.0 and 2.1 versions. Admonitions are included to indicate that 2.1 uses the 2.0 tests, while 2.2 tests are yet to be defined.
Pages updated:
This is a precursor to creating a version for 2.2 compliance (#1415). When creating that set, I believe we should withdraw the 1.2 tests from the docs pages (will exist as is in the version above) and focus on only the current test set. That has been applied to the 2.2 (/docs) versions already, ready for 2.2 tests to be added and labelled as new to that version.
I have not implemented the conformance test pages for 1.2 as they will require more editing to remove 2.0 tests. If that is needed in your opinion, let me know in a review.
N.B finos/FDC3-conformance-framework#264 should be merged after this to correct links in the confromance framework's readme to point to the new Overview page (replaces the README.md file)