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

filter by the context type we're checking for #194

Closed nkolba closed 1 year ago

nkolba commented 1 year ago

@robmoffat

kriswest commented 1 year ago

How come a typed listener is needed in these basic tests? An untyped listener should still receive the contexts broadcast via a null typed listener. There are filtered tests later in both sets...

nkolba commented 1 year ago

The issue is that in FDC3 implementations that retain history on channels, the current context on a wildcard listener can be contaminated by previous tests and produce failures. If we're in agreement that adding more precision here doesn't alter the meaning and purpose of the test, then is there harm in doing so?

kriswest commented 1 year ago

I was under the impression that this was already dealt with when we added the testId to the contexts so we could ignore all messages from outside the specific test...

I can see the testId being added but it looks like it's not being used in setupAndValidateListener where it is in other functions like waitForContext.

I think matching the testId is probably the better approach as you could still have collisions on a filtered type.

nkolba commented 1 year ago

That's fine with me. though I think for this test, it may be 6 one way / half-dozen the other. In this case, I believe it is just validating that its received a context of type 'fdc3.instrument'.

On Mon, Mar 20, 2023 at 5:14 PM Kris West @.***> wrote:

I was under the impression that this was already dealt with when we added the testId to the contexts so we could ignore all messages from outside the tests...

I can see the testId being added but it looks like it's not being used in setupAndValidateListener where it is in other functions like waitForContext.

I think matching the testId is probably the better approach as you could still have collisions on a filtered type.

— Reply to this email directly, view it on GitHub https://github.com/finos/FDC3-conformance-framework/pull/194#issuecomment-1476943444, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIY7GX2FA44FGLZZLYLOS3W5DCDHANCNFSM6AAAAAAWBERHVM . You are receiving this because you authored the thread.Message ID: @.***>

nkolba commented 1 year ago

@kriswest does the above explanation make sense to you?

robmoffat commented 1 year ago

Hi Nick,

Can we have some details on what is failing and why this is necessary? In order for @gaganahluwalia to make some changes to the code, we're going to need to have something that breaks and then be able to fix it. Does that make sense?

thanks, Rob

kriswest commented 1 year ago

@kriswest does the above explanation make sense to you?

It does (particularly as it was dealt with before on other tests - although not why it became a problem again as we've all passed those tests already? Hence, Rob asking for an example fail above - so they've got a clear thing to fix and test afterwards.

Took a look with Rob and Gagan this morning and explained the testId checks. I think it'll end up being the better solution as the order tests run in can change, we might add more tests in other versions etc.. Also, they need to make the changes on the 2.0-release-candidate-2 branch (implements 2.0 tests, reactors 1.2 a bit for code reuse between the tests).

nkolba commented 1 year ago

@robmoffat

the relevant code looks like this:

    const resolveExecutionCompleteListener =
        cc.initCompleteListener(scTestId1);
      let receivedContext = false;
      await cc.setupAndValidateListener1(
        null,
        null,
        "fdc3.instrument",
        errorMessage,
        () => (receivedContext = true)
      );
      const channel = await cc.retrieveAndJoinChannel(1);
      await cc.openChannelApp(scTestId1, channel.id, JOIN_AND_BROADCAST);
      await resolveExecutionCompleteListener;

This adds a context listener (with the validator for context type of 'fdc3.instrument') for any context type and then joins a channel (e.g. 'red'). When the channel is joined, whatever the most recent context is will be pushed to the handler - regardless of type. If the type isn't the expected 'fdc3.instrument', the test fails.

Connectifi will save channel history in a session. So, if you rerun the tests in the same session, a test after this one has set a different context type on the same channel. Which means that when this test runs it will receive the unexpected context on first joining the channel - resulting in a failure. These tests fail consistently if there is previous session state, reseting the session consistently results in a pass.

Can't really speak to why these tests aren't failing on other systems or why they started failing. I believe with Sail, we we're probably starting the runtime fresh with each test run. Its also possible that different systems have different handling for sending context on channel join when a listener hasn't specified a context type.

kriswest commented 1 year ago

Ah, multiple runs of the tests without a reset, that'd explain it.

kriswest commented 1 year ago

This has been superseded by #195 (merged to main). @nkolba you could test that again and close this if the problem is resolved.

nkolba commented 1 year ago

thanks @kriswest just tested against main and the tests pass now.