finos / FDC3-conformance-framework

A framework for testing whether desktop containers implement the FDC3 standard
Apache License 2.0
14 stars 15 forks source link

Bugfixes: FDC3 2.0 Conformance OSFF 2024 #260

Closed jupnit closed 1 month ago

jupnit commented 1 month ago
  1. 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.

  2. 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.

  3. 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.

linux-foundation-easycla[bot] commented 1 month ago

CLA Signed

The committers listed above are authorized under a signed CLA.

robmoffat commented 1 month ago

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.

kriswest commented 1 month ago

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.

A Desktop Agent implementation should ensure that is not required. addContextListener and addIntentlistener are async functions that should not resolve until the Desktop Agent has done the required setup. However, this ridiculous implementation causes a race as the 'receiveContext' function is both setting up the listener and then resolving when it receives something. The steps should be separated to ensure correct control flow rather than tacking in sleeps to work around the race.

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

robmoffat commented 1 month ago

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.

kriswest commented 1 month ago

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.

The forthcoming 2.2 conformance pack offers us an opportunity to wipe the slate clean and sort all this out properly.

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.

jupnit commented 1 month ago

EasyCLA

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.

@robmoffat The authorization was granted.