-
Notifications
You must be signed in to change notification settings - Fork 18
Bugfixes: FDC3 2.0 Conformance OSFF 2024 #260
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
Bugfixes: FDC3 2.0 Conformance OSFF 2024 #260
Conversation
robmoffat
left a comment
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!
|
Thanks @jupnit - you'll need to go through the EasyCLA workflow above. If you have any issues with this, reach out to help@finos.org and they'll assist. |
A Desktop Agent implementation should ensure that is not required. In short, I agree there is a problem here - but it needs to be solved with a deeper refactor to fix the terrible code. Thanks for pointing out the flaw @jupnit |
|
Hi @kriswest, We're probably on the same page on this - the conformance framework as-is is straining to do its job, and is a rats-nest of promises and race conditions. The forthcoming 2.2 conformance pack offers us an opportunity to wipe the slate clean and sort all this out properly. |
|
If reworking the test implementations then a guiding principle should be NOT mixing async/await with manual promise declarations/nested promises. Even if done correctly it creates unnecessary cognitive complexity. Looking at it, this implementation is inside out. The test timeout and resolution in the handler should be set up outside the util function in teh main part of the test, while the util should be a simple async fn that awaits creating the channel, adds the listener, then broadcasts the message. Perhaps just dump the utils.
Agreed, the longer I stare at it the more convinced I am that we should start over from the test definitions and write something both comprehensible and maintainable. |
@robmoffat The authorization was granted. |
v2.0/advanced/fdc3.findIntent.ts: Fixed the displayName for Intent.sharedTestingIntent2 and Intent.sharedTestingIntent1 to 'Shared Testing Intent 2' and 'Shared Testing Intent 1', respectively.
v2.0/advanced/fdc3.raiseIntent.ts: Suggesting adding a brief delay of ~300ms at line 118 after the context listeners are added, and before broadcasting the context to private channel, to ensure the Desktop Agent has sufficient time to initialize everything properly.
Manual Tests: The TestTimeout of 20,000ms seemed insufficient for fdc3ResolveAmbiguousContextTargetMultiInstance_2_0 and fdc3ResolveAmbiguousIntentTargetMultiInstance_2_0. These tests open four applications, then trigger a UI resolver app, which waits for user input before resolving. Given the dependency on user action, suggesting that the timeout should be extended.