Fix the "must check that an infinite loop is not triggered" integration test #19064
Conversation
…on test This integration test fails intermittently, locally at least in Chrome with Puppeteer 23.4.0+, with the following errors: ``` In chrome: Expected '123Hello' to equal 'Hello123'. In chrome: Expected '123Hello' to equal '123'. ``` This happens because the test before it left queued sandbox events behind. We don't close the document between tests, so those get run when we click the textbox in this test and that interferes with our selection/typing actions. This commit fixes the issue by flushing the queued sandbox events in the first test, which makes sure that state no longer leaks through to the next test and thus improves isolation. Morever, similar to commit 3adf8b6 we use safer assertions to avoid further intermittent failures, and we replace the `page.$eval` call with a simpler Home button push like we already do in e.g. the test helpers. This combined makes the code shorter and simpler.
|
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/0cbf1cfbc417644/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/6970813f0df0b06/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/0cbf1cfbc417644/output.txt Total script time: 8.83 mins
|
Possibly a stupid question, but could/should we just close the document between tests instead? |
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/6970813f0df0b06/output.txt Total script time: 19.36 mins
|
It's a good question actually, and I asked myself that one as well while making the patch. I did consider that, but because we don't seem to do that anywhere else yet I figured I'd go for consistency here. Moreover, in general it seems like good practice to have the tests clean up anything they created themselves. However, I do agree that we might want to consider doing that in follow-up work. Isolation issues (dependencies between tests) are currently not visible because we run the integration tests in a fixed order, in contrast to e.g. the unit tests which already run in a random order. I think fixing that will bring any remaining isolation issues to the surface, and fixing them will then probably result in having the close the document between tests. For now though the objective is to unblock Puppeteer updates, so I therefore kept this patch small to fix one thing at a time, but I have created follow-up issue #19065 to track this because I do think that it is a good idea (but probably also quite a bit of work similar to what we had to do to get the intermittents under control). |
Yes we should make all the tests independent of each other, mainly because it's super painful to have to run all test before the one you want to debug... |
This integration test fails intermittently, locally at least in Chrome with Puppeteer 23.4.0+, with the following errors:
This happens because the test before it left queued sandbox events behind. We don't close the document between tests, so those get run when we click the textbox in this test and that interferes with our selection/typing actions. This commit fixes the issue by flushing the queued sandbox events in the first test, which makes sure that state no longer leaks through to the next test and thus improves isolation.
Morever, similar to commit 3adf8b6 we use safer assertions to avoid further intermittent failures, and we replace the
page.$evalcall with a simpler Home button push like we already do in e.g. the test helpers. This combined makes the code shorter and simpler.Fixes a part of #18773 (the scripting tests now all pass for me locally with the new Puppeteer version).