Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the copy/paste logic in the integration tests and fix a race condition involving the waitForEvent integration test helper function #18331

Merged

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Jun 25, 2024

The commit messages contain more details about the individual changes.

Note that this patch should fix the waitForEvent timeout logs (but not the waitForEvent validation logs yet) and reduce the likelyhood of the FreeText Editor Paste some html must check that pasting html just keep the text integration test, and other related integration tests, failing (but not completely fix it yet). I'm fairly sure I now know how to fix it completely, but that'll be done in a follow-up which builds on top of this patch and the previous patches.

Fixes a part of #17931.

@timvandermeij timvandermeij changed the title Refactor the copy/paste logic in the integration tests and fix a race condition involving the waitForEvent integration test helper function Refactor the copy/paste logic in the integration tests and fix a race condition involving the waitForEvent integration test helper function Jun 25, 2024
@timvandermeij timvandermeij force-pushed the integration-test-copy-pasting branch from 4ca71a8 to ae4e8d5 Compare June 25, 2024 13:11
@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 1

Live output at: http://54.241.84.105:8877/a8bd6379e9f108b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 1

Live output at: http://54.193.163.58:8877/ecc6f7761afddf0/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/a8bd6379e9f108b/output.txt

Total script time: 7.72 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/ecc6f7761afddf0/output.txt

Total script time: 19.34 mins

  • Integration Tests: FAILED

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/8b2d77b3699e0cb/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/e6303dacd1d3a72/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/e6303dacd1d3a72/output.txt

Total script time: 8.13 mins

  • Integration Tests: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/8b2d77b3699e0cb/output.txt

Total script time: 18.78 mins

  • Integration Tests: Passed

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/270824e9e72aaca/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/4781d821f5a0b5f/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/4781d821f5a0b5f/output.txt

Total script time: 7.78 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/270824e9e72aaca/output.txt

Total script time: 19.89 mins

  • Integration Tests: FAILED

@timvandermeij timvandermeij marked this pull request as ready for review June 25, 2024 14:58
@timvandermeij timvandermeij force-pushed the integration-test-copy-pasting branch from ae4e8d5 to 488e47b Compare June 25, 2024 17:14
@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/68085c03b258a42/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/4fce05545aa2f20/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/68085c03b258a42/output.txt

Total script time: 8.14 mins

  • Integration Tests: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/4fce05545aa2f20/output.txt

Total script time: 19.06 mins

  • Integration Tests: FAILED

@timvandermeij timvandermeij force-pushed the integration-test-copy-pasting branch 2 times, most recently from 0e1997d to 385550f Compare June 25, 2024 19:17
@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/107b5a7a5acaa86/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/6d478446cda4323/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/6d478446cda4323/output.txt

Total script time: 7.76 mins

  • Integration Tests: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/107b5a7a5acaa86/output.txt

Total script time: 18.42 mins

  • Integration Tests: Passed

The integration tests are currently not consistent in how they do
copy/pasting: some tests use the `kbCopy`/`kbPaste` functions with
waiting for the event inline, some have their own helper function to
combine those actions and some even call `kbCopy`/`kbPaste` without
waiting for the event at all (which can cause intermittent failures).

This commit fixes the issues by providing a set of four helper functions
that all tests use and that abstract e.g. waiting for the event away
from the caller. This makes the invididual tests simpler and consistent,
reduces code duplication and fixes possible intermittent failures
due to not waiting for events to trigger.
…lper function

Debugging mozilla#17931 uncovered a race condition in the way we use the
`waitForEvent` function. Currently the following happens:

1. We call `waitForEvent`, which starts execution of the function body
   and immediately returns a promise.
2. We do the action that triggers the event.
3. We await the promise, which resolves if the event is triggered or
   the timeout is reached.

The problem is in step 1: function body execution has started, but not
necessarily completed. Given that we don't await the promise, we
immediately trigger step 2 and it's not unlikely that the event we
trigger arrives before the event listener is actually registered in the
function body of `waitForEvent` (which is slower because it needs to be
evaluated in the page context and there is some other logic before the
actual `addEventListener` call).

This commit fixes the issue by passing the action to `waitForEvent` as
a callback so `waitForEvent` itself can call it once it's safe to do so.
This should make sure that we always register the event listener before
triggering the event, and because we shouldn't miss events anymore we
can also remove the retry logic for pasting.
@timvandermeij timvandermeij force-pushed the integration-test-copy-pasting branch from 385550f to 7128b95 Compare June 26, 2024 13:38
@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/b791c8df3a640d7/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/be8cb320ef713bf/output.txt

@timvandermeij timvandermeij requested a review from calixteman June 26, 2024 13:41
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/b791c8df3a640d7/output.txt

Total script time: 7.75 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/be8cb320ef713bf/output.txt

Total script time: 18.82 mins

  • Integration Tests: FAILED

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@timvandermeij timvandermeij merged commit 6d57908 into mozilla:master Jun 26, 2024
7 checks passed
@timvandermeij timvandermeij deleted the integration-test-copy-pasting branch June 26, 2024 14:21
@timvandermeij timvandermeij removed the request for review from Snuffleupagus June 26, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants