search

agnivade / wasmbrowsertest

Run WASM tests inside your browser
MIT License
191 stars 22 forks source link

Do not cancel context on exception #65

Closed agnivade closed 4 months ago

agnivade commented 4 months ago

Sometimes, exception can happen if a callback runs after the test has exited. In that case, cancelling a context fails the test which is not intentional.

Since cancelling happens anyways after a test exits, there is not much point manually cancelling it and we try to keep parity with non-browser environments.

Fixes #60

agnivade commented 4 months ago

@hajimehoshi @paralin - Apologies for the delay on this. Wondering if you would be able to give this a try in your CI envs and let me know if this fixes it. Thanks!

hajimehoshi commented 4 months ago

I am testing this now: https://github.com/hajimehoshi/ebiten/actions/runs/10211074516

agnivade commented 4 months ago

Hmm .. it seems like this issue is fixed, but this seems to have appeared on Ubuntu now? Has this happened before? I've never seen it fail on Linux before.

hajimehoshi commented 4 months ago

This failed due to "websocket url timeout reached" unfortunately 🤔

agnivade commented 4 months ago

That is what I mentioned in my comment before. This is #59 but we have only seen this in Windows before. Has this failed in Linux before? Or is this the first time?

hajimehoshi commented 4 months ago

Not sure but I think this happened on Linux before

paralin commented 4 months ago

I will test it

hajimehoshi commented 4 months ago

Now the tests passed. I guess the situation gets much better than before!

agnivade commented 4 months ago

Cool. Looks like this issue is fixed. The "websocket url timeout" is a separate issue which also needs to be looked into. Thanks for testing this out Hajime san!

I will wait for @paralin's confirmation before merging this.

paralin commented 4 months ago

I think this might still be an instance of the error: https://github.com/aperturerobotics/bifrost/actions/runs/10213182411/job/28258052721#step:14:527

That one is not a context canceled but rather "Go program has already exited" - not sure if this pr was intended to fix that or not?

agnivade commented 4 months ago

No my PR does not fix that. That error appears from the wasm_exec.js, but the real thing that actually fails the test is the cancelling of context. From your test output, it appears that the test has passed successfully.

I think it's good to keep the output to give an indication that some callback has run after the test has finished, to give an opportunity for the developer to fix the issue. But it does the fail the test.

paralin commented 4 months ago

@agnivade Ah, alright. Well I would prefer to not have that error, because it is not something I can fix - it's not really possible to ensure no callback will happen after the program exits.

agnivade commented 4 months ago

The error comes from here. Arguably it could be removed, but IMO it's there as an indicator to the user that something unexpected has happened. As you said, sometimes it might not really be possible, but sometimes it might be. Atleast in those cases, the user should know that there's something to be fixed.