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

Open tests v2 + refactor all open tests #140

Closed Joe-Dunleavy closed 1 year ago

Joe-Dunleavy commented 1 year ago

Implements V2 open tests + refactors v1 & v2 open tests

Notes:

kriswest commented 1 year ago

@Joe-Dunleavy I'm not sure why Finsemble it is immediately rejecting (possibly due to interop metadata not containing the intent - I was expecting a timeout to apply - which i think it does in our current pre-release). However (and as discussed at the last maintainers meeting) the OpenError.AppTimeout error (and ResolveError.IntentDeliveryFailed which is similar) are both specified in the docs and sources:

enum OpenError {
  ...
  /** Returned if the specified application launches but fails to add a context
   *  listener in order to receive the context passed to the `fdc3.open` call.
   */
  AppTimeout = 'AppTimeout',

https://fdc3.finos.org/docs/api/ref/Errors#openerror

export enum ResolveError {
  ...
  /** Returned if the intent and context could not be delivered to the selected
   *  application or instance, for example because it has not added an intent
   *  handler within a timeout.
   */
  IntentDeliveryFailed = 'IntentDeliveryFailed',
}

it was agreed at the meeting to drop the testing of these in 1.2 (as poorly documented) but to retain it in 2.0 (as it is documented). AOpensBNoListenTest is one of the tests dropped in 1.2, AOpensBWithWrongContextTest is the other. AFailsToOpenB2-3 should both be implemented in 1.2 tests... However, AFailsToOpenB1-3 all differ in 2.0 as signatures based on name were deprecated (so only the new signature based on AppIdentifier needs to be tested)

We may seek to make the docs more explicit/easier to find in future by adding something to the relevant function reference entries.

robmoffat commented 1 year ago

This is a pretty big change - I am tempted to say we should leave this change for after we've completed the 1.2 conformance badging (big assumption I know).

I'm glad you've done it, but since it (should) only materially impact 2.0 I think we should defer the merge.

wdyt?

Joe-Dunleavy commented 1 year ago

@robmoffat I need to create a separate PR that removes those two tests that Kris mentioned above from 1.2 if you'd rather not merge this in straight away. Aside from that, the changes here only affect 2.0 tests. Changes to 1.2 open tests are cosmetic.

robmoffat commented 1 year ago

@ksgeorgieva - please could you run this PR against Glue and check that there are no regressions for the 1.2 tests.