Revert "Use context cancellation for signaling that browser process is done" #599
Conversation
|
grafana/k6#4417 happened again 😞 Maybe the whole thing is racy? I really don't see how anything else in #598 could be causing this. Unless it's GitHub being flaky again. 🤷♂️ Any suggestions? |
Can you run the CI job several times, and compare those results with running the CI against I think we should reopen the issue, and dedicate some time in the next cycle to it -- key thing being trying to reproduce it locally. |
|
Hey @imiric, I haven't reviewed this yet because it seems one of the E2E tests is failing. What's the plan for this PR? |
|
The plan was to see if reverting e35e88c would improve the flakiness situation, but judging by the CI failure, I'm not sure it did. Plus I got sidetracked looking into grafana/k6#4417 instead. Don't worry about this for now, we can decide in a few days what to do with it. |
…s done" This reverts commit e35e88c. It seems the previous change might've been causing some flakiness issues. See https://github.com/grafana/xk6-browser/issues/566#issuecomment-1285672600
imiric
force-pushed
the
revert/598-partial
branch
from
a798499 to
0424769
Compare
|
I needed this for #617, and I think it's overall safer than the previous context cancellation approach, so @inancgumus you can review it now. Although it's just a reversion; it shouldn't have any effect on the existing flakiness. |
This reverts commit e35e88c.
It seems the previous change might've been causing some flakiness issues. See grafana/k6#4417 and #598 (comment) .
Using a context instead of a channel was cleaner, and while on the surface shouldn't have any functional differences, the context cancellation might be time sensitive, so a channel should be safer.