search

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

Mock app control messages break tests in some cases #83

Closed kriswest closed 2 years ago

kriswest commented 2 years ago

We've run into an issue with the tests while testing that we think will affect most other desktop agents and needs fixing in the test framework:

The broadcast/User channel tests are using the system/user channels to send control messages to the mock apps, specifically a 'close' message here:

However, if the channel retains context (which many implementations do), that context will already be present and will be received in the second broadcast test, breaking it (and no doubt a few others). I.e. it will immediately be received here:

You can replicate this by opening another component and putting it on the first system/user channel before you start the test as this will stop Finsemble draining the channel of state:

image

To fix, move these messages off of the system channels and onto a dedicated app channel (which the mocks should always independently listen for).

@ColinEberhardt @Joe-Dunleavy

Joe-Dunleavy commented 2 years ago

Hi Kris, I will address this asap.

kriswest commented 2 years ago

That only happens if all apps leave and it's an implementation decision in Finsemble, rather than the standard itself, which doesn't comment on how long context should or shouldn't stay on channels.

Lemme know how you get on. We're keen to run another test!

K

On Tue, 25 Oct 2022, 08:53 Joe Dunleavy, @.***> wrote:

Hi Kris, I will address this asap. My understanding was that the broadcast context was dropped if the app that is broadcasting leaves the channel -- as is the case between each test.

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

Joe-Dunleavy commented 2 years ago

@kriswest I've updated my pull request with a fix for this issue.

All messaging between apps related to closing mock app windows and checking if mock app execution has been completed is now done over a single dedicated app channel named "app-control"

In addition, the changes I made to the channel tests with regard to using window.close for closing app windows instead of using Finsemble's implementation have been carried over to all other tests.

Also, the timeout that waits for the mock app windows to close has now been removed for all tests -- not just the channel tests.

I'm currently on holiday until Monday so I won't be available until then, but that pull request should resolve all of the most pressing issues.

kriswest commented 2 years ago

@Joe-Dunleavy great that does indeed resolve the issue.

However, I've hit a different one it has exposed. There is a race between the execution-complete messages and the listeners being added for them. Have commented on the PR on an example and possible solution.

window.close isn't working for me in BrowserView windows in a current Finsemble release (a regression we're working on) if that does affect you can tell via the central logger (are the invisible mock apps closing?). To workaround that add manifest.foreign.components.Window Manager.titlebarType = "injected" to the config of each mock.

Joe-Dunleavy commented 2 years ago

@kriswest I was able to resolve the issue with windows not closing when using window.close when you mentioned the workaround in a previous comment.

Do you still want me to look into the issue regarding the "race between the execution-complete messages and the listeners being added for them" or has that since been resolved?

kriswest commented 2 years ago

Take a look at the open PR and add a review if you would - its recreating your PR + my changes. The race seems solved for me - I had to make the handling of the execution messages dependent on the test name to resolve (so it can ignore one from a previous test if it needs to).

https://github.com/finos/FDC3-conformance-framework/pull/85

On Mon, 31 Oct 2022 at 09:57, Joe Dunleavy @.***> wrote:

@kriswest https://github.com/kriswest I was able to resolve the issue with windows not closing when using window.close when you mentioned the workaround in a previous comment.

Do you still want me to look into the issue regarding the "race between the execution-complete messages and the listeners being added for them" or has that since been resolved?

— Reply to this email directly, view it on GitHub https://github.com/finos/FDC3-conformance-framework/issues/83#issuecomment-1296854367, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM7PBBJRJXZ4GPU24QDTKTWF6JZDANCNFSM6AAAAAARNHZAVQ . You are receiving this because you were mentioned.Message ID: @.***>

-- Kris West Principal Engineer [image: Finsemble] https://cosaic.io/finsemble +1 (800) 821-8147 @.***