Conformance additions for 2.2 (second attempt) #1426
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.
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.
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.
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.
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.
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.
@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.
(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 <52539182+kemerava@users.noreply.github.com>
|
/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 |
|
@kriswest I've added some a few comments for some minor formatting / consistency changes. I believe the earlier changes you made look good though. |
|
Many thanks @novavi, just one approving @finos/fdc3-maintainers review to go on this one! |
| - `TargetedResolve1`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, <A"s App Name>)` to start app A, otherwise, as above | ||
| - `TargetedResolve2`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, {name: "<A's App Name>"})` to start app A, otherwise, as above | ||
| - `TargetedResolve3`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, {name: "<app B Name>", appId: "<app B ID>"})` to start app B, otherwise, as above | ||
| - `FailedResolve1-3` As with `TargetedResolve1-3`, but use `fdc3.raiseIntent("aTestingIntent", {testContextY}, <A"s App Name>)` and variations. You will receive `NoAppsFound` Error | ||
| - `FailedResolve4` As above, but use `fdc3.raiseIntent("aTestingIntent", {testContextX}, <C"s App Name>)`. You will receive `NoAppsFound` Error |
There was a problem hiding this comment.
I think there are some places where ' is still needed (and some inconsistency):
| - `TargetedResolve1`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, <A"s App Name>)` to start app A, otherwise, as above | |
| - `TargetedResolve2`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, {name: "<A's App Name>"})` to start app A, otherwise, as above | |
| - `TargetedResolve3`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, {name: "<app B Name>", appId: "<app B ID>"})` to start app B, otherwise, as above | |
| - `FailedResolve1-3` As with `TargetedResolve1-3`, but use `fdc3.raiseIntent("aTestingIntent", {testContextY}, <A"s App Name>)` and variations. You will receive `NoAppsFound` Error | |
| - `FailedResolve4` As above, but use `fdc3.raiseIntent("aTestingIntent", {testContextX}, <C"s App Name>)`. You will receive `NoAppsFound` Error | |
| - `TargetedResolve1`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, <A's App Name>)` to start app A, otherwise, as above | |
| - `TargetedResolve2`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, {name: "<A's App Name>"})` to start app A, otherwise, as above | |
| - `TargetedResolve3`: Use `fdc3.raiseIntent("aTestingIntent", {testContextX}, {name: "<B's App Name>", appId: "<B's App ID>"})` to start app B, otherwise, as above | |
| - `FailedResolve1-3` As with `TargetedResolve1-3`, but use `fdc3.raiseIntent("aTestingIntent", {testContextY}, <A's App Name>)` and variations. You will receive `NoAppsFound` Error | |
| - `FailedResolve4` As above, but use `fdc3.raiseIntent("aTestingIntent", {testContextX}, <C's App Name>)`. You will receive `NoAppsFound` Error |
| @@ -130,7 +130,7 @@ Finally, please note that this is a larger set of apps than were required for 1. | |||
|
|
|||
| - `2.0-RaiseIntentSingleResolve`: Perform above test | |||
| - `2.0-RaiseIntentTargetedAppResolve`: Repeat the above test, but: | |||
| - In the first step use `fdc3.raiseIntent("sharedTestingIntent1", testContextX, {"appID": "<B’s appId>"})` to start app B, | |||
| - In the first step use `fdc3.raiseIntent("sharedTestingIntent1", testContextX, {"appID": "<B"s appId>"})` to start app B, | |||
There was a problem hiding this comment.
| - In the first step use `fdc3.raiseIntent("sharedTestingIntent1", testContextX, {"appID": "<B"s appId>"})` to start app B, | |
| - In the first step use `fdc3.raiseIntent("sharedTestingIntent1", testContextX, {"appID": "<B's appId>"})` to start app B, |
| @@ -234,7 +234,7 @@ Finally, please note that this is a larger set of apps than were required for 1. | |||
|
|
|||
| | App | Step | Details | | |||
| |-------|-----------------|---------------------------------------------------------------------------------------------------| | |||
| | Test | 1. Raise intent | Test raises an intent with `fdc3.raiseIntent(‘"kTestingIntent", testContextX, {appId: "<K's appId>"})`<br />starts app K. | | |||
| | 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.
| | Test | 1. Raise intent | Test raises an intent with `fdc3.raiseIntent(""kTestingIntent", testContextX, {appId: "<K's appId>"})`<br />starts app K. | | |
| | Test | 1. Raise intent | Test raises an intent with `fdc3.raiseIntent("kTestingIntent", testContextX, {appId: "<K's appId>"})`<br />starts app K. | |
| | Test | 2.Confirm | Compare the `AppMetadata` object to the expected definition for the fields provided above during setup and ensure that the metadata matches. An `instanceId` should NOT be set | | ||
| | App | Step | Details | | ||
| |--------|-------------------|---------------------------------------------------------------------------------------------------| | ||
| | Test | 1. getAppMetadata | Retrieve metadata for the configured app A with <br/> `const metadata1 = await fdc3.getAppMetadata({appId:"<A's-appId>"})` | |
There was a problem hiding this comment.
is this - needed in {appId:"<A's-appId>"}?
| | Test | 1. Open1 | Open the first instance of App A using <br/> `const appIdentifier1 = await fdc3.open({appId:"<A's-appId>"})` <br/>and confirm that its `AppIdentifier` contains an `instanceId`. | | ||
| | Test | 2. Open2 | Open a second instance of App A using <br />`const appIdentifier2 = await fdc3.open({appId:"<A's-appId>"})` <br/>and confirm that its `AppIdentifier` contains an `instanceId` and that its value differs from that returned for the first instance. | | ||
| | Test | 3. FindInstances | Retrieve details of open instances of app A with <br/> `let instances = await fdc3.findInstances({appId:"<A's-appId>"})` <br/> confirm that both `appIdentifier1` and `appIdentifier2` are both present in the array. | |
There was a problem hiding this comment.
Is - needed in ({appId:"<A's-appId>"})?
resolves #1415
supersedes #1271
Adds conformance tests for FDC3 2.2.
Incorporates changes from/follows on from #1417.
Review deep links: