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

Fix the "must check that charLimit is correctly set" scripting integration test #18399

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

timvandermeij
Copy link
Contributor

This integration test fails intermittently because we're not (correctly) awaiting the character limit increase sandbox action.

For the first increase an attempt was made to handle this, but it doesn't work correctly because the text in the field is abcdefghijklmnopq and it's not be truncated until the sandbox action is completed, so because we didn't await that we would could pass the value !== "abcdefgh" check because abcdefghijklmnopq !== abcdefgh is true. For the second increase we didn't have a check in place.

This commit fixes the issues by using the waitForSandboxTrip and waitForSelector helper functions to make sure that we can only proceed if the sandbox action is completed and the DOM element is updated.

Fixes a part of #18396.

…ation test

This integration test fails intermittently because we're not (correctly)
awaiting the character limit increase sandbox action.

For the first increase an attempt was made to handle this, but it doesn't
work correctly because the text in the field is `abcdefghijklmnopq` and
it's not be truncated until the sandbox action is completed, so because
we didn't await that we would could pass the `value !== "abcdefgh"` check
because `abcdefghijklmnopq !== abcdefgh` is true. For the second increase
we didn't have a check in place.

This commit fixes the issues by using the `waitForSandboxTrip` and
`waitForSelector` helper functions to make sure that we can only proceed
if the sandbox action is completed and the DOM element is updated.
@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/39a39be7d1df13e/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/2b13873ed2945db/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/39a39be7d1df13e/output.txt

Total script time: 7.93 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/2b13873ed2945db/output.txt

Total script time: 16.68 mins

  • Integration Tests: Passed

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 7a5f561 into mozilla:master Jul 5, 2024
7 checks passed
@timvandermeij timvandermeij deleted the intermittent-charlimit branch July 5, 2024 19:58
@timvandermeij timvandermeij removed the request for review from Snuffleupagus July 5, 2024 19:58
timvandermeij added a commit to timvandermeij/pdf.js that referenced this pull request Nov 3, 2024
This integration test fails intermittently because we're not
(correctly) awaiting the sandbox actions. The `27R` field in
`issue14862.pdf` triggers sandbox events for every typing action, but
for the backspace and "a" character typing actions we weren't awaiting
the sandbox trip at all, and for other places we weren't awaiting it
fully (causing some characters to be missed in the assertion).

This commit fixes the issues by using the appropriate helper functions,
similar to what we did in PR mozilla#18399.
timvandermeij added a commit to timvandermeij/pdf.js that referenced this pull request Nov 3, 2024
This integration test fails intermittently because we're not
(correctly) awaiting the sandbox actions. The `27R` field in
`issue14862.pdf` triggers sandbox events for every typing action, but
for the backspace and "a" character typing actions we weren't awaiting
the sandbox trip at all, and for other places we weren't awaiting it
fully (causing some characters to be missed in the assertion).

This commit fixes the issues by using the appropriate helper functions,
similar to what we did in PR mozilla#18399. Not only is this shorter in terms
of code, but it also fixed the near-permafail for this test with newer
versions of Puppeteer.
timvandermeij added a commit to timvandermeij/pdf.js that referenced this pull request Nov 3, 2024
… integration test

This integration test fails intermittently because we're not correctly
awaiting typing actions, causing some characters to be missed in the
assertion. This commit fixes the issues by directly waiting for the
expected text area contents, similar to e.g. PR mozilla#18399 and PR mozilla#19001.
ryzokuken pushed a commit to ryzokuken/pdf.js that referenced this pull request Nov 4, 2024
This integration test fails intermittently because we're not
(correctly) awaiting the sandbox actions. The `27R` field in
`issue14862.pdf` triggers sandbox events for every typing action, but
for the backspace and "a" character typing actions we weren't awaiting
the sandbox trip at all, and for other places we weren't awaiting it
fully (causing some characters to be missed in the assertion).

This commit fixes the issues by using the appropriate helper functions,
similar to what we did in PR mozilla#18399. Not only is this shorter in terms
of code, but it also fixed the near-permafail for this test with newer
versions of Puppeteer.
timvandermeij added a commit to timvandermeij/pdf.js that referenced this pull request Dec 7, 2024
The `must check input for US zip format` integration test fails pretty
consistently in Puppeteer 23.4.0+ with `Expected '12341' to equal
'12345'`. This is reproducible with the `pdf.sandbox.external.js` hack
from mozilla#18396 (comment).
Investigation uncovered two issues at play here:

1. We do two `clearInput` calls, but don't await processing of the two
   sandbox events that are triggered by that action. The three tests that
   use `issue14307.pdf` are in different `describe` blocks and therefore
   reload the PDF file, so we can simply remove those calls because the
   inputs are already empty by default.

2. We don't await processing of the sandbox events that occur after
   switching to another text field. This causes the expectation failure
   because the typing actions will happen too soon and interfere with
   the sandbox event processing. We solve the issue by explicitly
   awaiting the sandbox roundtrip.

Moreover, similar to PR mozilla#19001 and mozilla#18399 we remove any remaining room
for intermittent issues by directly checking for the expected value,
which also results in shorter code.
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