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 #1426

Merged
merged 24 commits into from
Feb 26, 2025
Merged

Conformance additions for 2.2 #1426

merged 24 commits into from
Feb 26, 2025

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 bdb4afa
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/67acc30ca9849e0008a0e4b8
😎 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?

| 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

@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
@novavi
Copy link

novavi commented Jan 30, 2025

@kriswest I've added some a few comments for some minor formatting / consistency changes. I believe the earlier changes you made look good though.

@kriswest
Copy link
Contributor Author

@novavi I've applied the comments from your review on #1417 here (as that one is merged). Are you able to approve this PR?

@novavi
Copy link

novavi commented Jan 30, 2025

@novavi I've applied the comments from your review on #1417 here (as that one is merged). Are you able to approve this PR?

@kriswest Apologies, not sure how they ended up on there... I did start from #1426 ! Yes, will approve now.

novavi
novavi previously approved these changes Jan 30, 2025
Copy link

@novavi novavi left a comment

Choose a reason for hiding this comment

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

Look good to me now

@kriswest
Copy link
Contributor Author

Many thanks @novavi, just one approving @finos/fdc3-maintainers review to go on this one!

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.

@kriswest Thanks for the changes you made, I was wondering also outside of the minor comments I left, if you think we should align were we use A's App Name vs app B Name vs app B ID vs K's appId?

@kriswest
Copy link
Contributor Author

kriswest commented Feb 5, 2025

@kemerava

I was wondering also outside of the minor comments I left, if you think we should align were we use A's App Name vs app B Name vs app B ID vs K's appId?

All API calls that used app name directly were deprecated in 2.0, e.g. https://fdc3.finos.org/docs/next/api/ref/DesktopAgent#raiseintent-deprecated as the name is potentially non-unique within even a single appD. They will still exist in the API definitions until an FDC3 3.0, however, conformance testing is not required (as deprecated API calls are optional - although they could be tested if present, however that may prove complex due to the function signature overloading).

The conformance test docs for 2.0 and 2.1 still include definitions for the 1.2 conformance tests (as a single set of docs served for them all). With this PR I've removed them from the 2.2 version (i.e. the files with paths starting _docs/api/conformance). Hence, there should be no tests in those files with an AppIdentifier that contains a name - I believe that is the case but would appreciate confirmation via review (if you're up for checking - I just did but two sets of eyes are better than one).

@kriswest
Copy link
Contributor Author

kriswest commented Feb 5, 2025

@kemerava apologies I may have misinterpreted your comment (as app name vs app Id), where you may have been referring to the format of the placeholder... There is some inconsistency in the form used. I'll leave it to you to confirm which you meant (or both) and to decide whether you want to have a crack at standardising. If you do I might suggest we swap roles and have you update the branch, close this PR, open a new one and I'll review. The branch protection rule will prevent whoever had the last push on the branch from approving the PR - but if thats you then I can approve. Up to you!

@bingenito
Copy link
Member

@kriswest and @kemerava I think I resolved the merge conflict which was only in the CHANGELOG

Copy link

504 passed

Copy link

Coverage Report

Commit: bdb4afa
Base: main@97f9809

Type Base This PR
Total Statements Coverage  97.41%  97.41% (+0%)
Total Branches Coverage  86.53%  86.53% (+0%)
Total Functions Coverage  96.85%  96.85% (+0%)
Total Lines Coverage  97.43%  97.43% (+0%)
Details (changed files)
FileStatementsBranchesFunctionsLines
Details (all files)
FileStatementsBranchesFunctionsLines
packages/fdc3-agent-proxy/src/DesktopAgentProxy.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/index.ts 100% 100% 62.5% 100%
packages/fdc3-agent-proxy/src/apps/DefaultAppSupport.ts 88% 50% 100% 88%
packages/fdc3-agent-proxy/src/channels/DefaultChannel.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/channels/DefaultChannelSupport.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/channels/DefaultPrivateChannel.ts 97.29% 66.66% 100% 97.29%
packages/fdc3-agent-proxy/src/heartbeat/DefaultHeartbeatSupport.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/intents/DefaultIntentResolution.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/intents/DefaultIntentSupport.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/listeners/AbstractListener.ts 100% 60% 100% 100%
packages/fdc3-agent-proxy/src/listeners/DefaultContextListener.ts 100% 90% 100% 100%
packages/fdc3-agent-proxy/src/listeners/DefaultIntentListener.ts 100% 77.77% 100% 100%
packages/fdc3-agent-proxy/src/listeners/EventListener.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/listeners/HeartbeatListener.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/listeners/PrivateChannelEventListener.ts 93.33% 72.72% 100% 93.33%
packages/fdc3-agent-proxy/src/messaging/AbstractMessaging.ts 94.59% 100% 80% 94.59%
packages/fdc3-agent-proxy/src/util/AbstractFDC3Logger.ts 100% 94.11% 100% 100%
packages/fdc3-agent-proxy/src/util/Logger.ts 100% 100% 100% 100%
packages/fdc3-agent-proxy/src/util/throwIfUndefined.ts 100% 100% 100% 100%
packages/fdc3-get-agent/src/index.ts 100% 100% 28.57% 100%
packages/fdc3-get-agent/src/messaging/MessagePortMessaging.ts 100% 100% 100% 100%
packages/fdc3-get-agent/src/messaging/message-port.ts 97.43% 86.66% 100% 97.43%
packages/fdc3-get-agent/src/sessionStorage/DesktopAgentDetails.ts 97.36% 89.47% 100% 97.36%
packages/fdc3-get-agent/src/strategies/DesktopAgentPreloadLoader.ts 100% 81.25% 100% 100%
packages/fdc3-get-agent/src/strategies/FailoverHandler.ts 100% 76.47% 100% 100%
packages/fdc3-get-agent/src/strategies/HelloHandler.ts 94% 81.25% 100% 94%
packages/fdc3-get-agent/src/strategies/IdentityValidationHandler.ts 95.65% 73.33% 100% 95.65%
packages/fdc3-get-agent/src/strategies/PostMessageLoader.ts 98.48% 86.95% 100% 98.46%
packages/fdc3-get-agent/src/strategies/Timeouts.ts 100% 100% 100% 100%
packages/fdc3-get-agent/src/strategies/getAgent.ts 100% 100% 100% 100%
packages/fdc3-get-agent/src/ui/AbstractUIComponent.ts 97.14% 71.42% 100% 97.01%
packages/fdc3-get-agent/src/ui/DefaultDesktopAgentChannelSelector.ts 100% 75% 100% 100%
packages/fdc3-get-agent/src/ui/DefaultDesktopAgentIntentResolver.ts 100% 90% 100% 100%
packages/fdc3-get-agent/src/ui/NullChannelSelector.ts 100% 100% 100% 100%
packages/fdc3-get-agent/src/ui/NullIntentResolver.ts 100% 100% 66.66% 100%
packages/fdc3-get-agent/src/util/Logger.ts 100% 100% 100% 100%
packages/fdc3-get-agent/src/util/Uuid.ts 100% 100% 100% 100%
packages/fdc3-standard/src/index.ts 100% 100% 0% 100%
packages/fdc3-standard/src/api/Errors.ts 100% 100% 100% 100%
packages/fdc3-standard/src/api/GetAgent.ts 100% 100% 100% 100%
packages/fdc3-standard/src/api/Methods.ts 94.04% 84.05% 96.29% 95%
packages/fdc3-standard/src/api/RecommendedChannels.ts 100% 100% 100% 100%
packages/fdc3-standard/src/context/ContextType.ts 100% 100% 100% 100%
packages/fdc3-standard/src/intents/Intents.ts 100% 100% 100% 100%
packages/fdc3-standard/src/internal/contextConfiguration.ts 100% 100% 100% 100%
packages/fdc3-standard/src/internal/intentConfiguration.ts 100% 100% 100% 100%
packages/fdc3-standard/src/internal/typeHelpers.ts 100% 100% 100% 100%
toolbox/fdc3-for-web/fdc3-web-impl/src/BasicFDC3Server.ts 100% 100% 100% 100%
toolbox/fdc3-for-web/fdc3-web-impl/src/ServerContext.ts 100% 100% 100% 100%
toolbox/fdc3-for-web/fdc3-web-impl/src/directory/BasicDirectory.ts 96.87% 84.21% 100% 96.55%
toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/BroadcastHandler.ts 96.38% 86.41% 100% 96.12%
toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/HeartbeatHandler.ts 88.23% 71.87% 86.66% 90%
toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/IntentHandler.ts 95.6% 86.56% 100% 95%
toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/OpenHandler.ts 97.14% 86.84% 100% 97.14%
toolbox/fdc3-for-web/fdc3-web-impl/src/handlers/support.ts 100% 100% 100% 100%

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.

LGTM

@kriswest kriswest requested review from a team February 26, 2025 13:28
@kriswest kriswest changed the title Conformance additions for 2.2 (second attempt) Conformance additions for 2.2 Feb 26, 2025
Copy link
Contributor

@julianna-ciq julianna-ciq left a 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. Most changes relate to syntax. While there was some additions to the content, they made sense to me and were placed reasonably in the documentation.

@kriswest kriswest merged commit 8fcde04 into main Feb 26, 2025
11 checks passed
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
7 participants