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 highlight text in the right position" integration test #17893

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Apr 5, 2024

This commit implements the following improvements for the test:

  • Replace the hardcoded highlight padding with a dynamic calculation. It turns out that the amount of padding can differ per system, possibly due to e.g. local (font) settings, which could cause the test to fail. The test now no longer implicitly depends on the environment.
  • Remove the page offset subtraction. It's only needed for one assertion, and we can simply add the page offset there once.
  • Improve the "magic" numbers in the test. The number 5 is removed now that the padding calculation made it obsolete, the number 28 is changed to 30 because that's the actual value in the PDF data and the number 4 is explained a bit more to state that the space also counts as a glyph.
  • Annotate the steps and checks to improve readability of the test and to explain why certain calculations have to be performed.

Fixes #17843. The integration tests now consistently pass locally too 🎉

@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/894a45f1f9defa4/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/74bccdd10f08c9e/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/894a45f1f9defa4/output.txt

Total script time: 7.37 mins

  • Integration Tests: Passed

@timvandermeij timvandermeij marked this pull request as ready for review April 5, 2024 18:00
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/74bccdd10f08c9e/output.txt

Total script time: 20.82 mins

  • Integration Tests: Passed

@Snuffleupagus Snuffleupagus requested a review from calixteman April 8, 2024 17:29
@timvandermeij timvandermeij force-pushed the find-integration-test branch from 474a19e to bcd2760 Compare April 9, 2024 12:23
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 for fixing this.

This commit implements the following improvements for the test:

- Replace the hardcoded highlight padding with a dynamic calculation. It
  turns out that the amount of padding can differ per system, possibly
  due to e.g. local (font) settings, which could cause the test to fail.
  The test now no longer implicitly depends on the environment.
- Remove the page offset subtraction. It's only needed for one assertion,
  and we can simply add the page offset there once.
- Improve the "magic" numbers in the test. The number 5 is removed now
  that the padding calculation made it obsolete, the number 28 is changed
  to 30 because that's the actual value in the PDF data and the number 4
  is explained a bit more to state that the space also counts as a glyph.
- Annotate the steps and checks to improve readability of the test and
  to explain why certain calculations have to be performed.
@timvandermeij timvandermeij force-pushed the find-integration-test branch from bcd2760 to 253a733 Compare April 11, 2024 11:42
@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/51f66a2d54d223f/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/6e3ec651cf6b812/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/51f66a2d54d223f/output.txt

Total script time: 7.44 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/6e3ec651cf6b812/output.txt

Total script time: 22.42 mins

  • Integration Tests: FAILED

@timvandermeij timvandermeij merged commit fcb76a7 into mozilla:master Apr 11, 2024
7 checks passed
@timvandermeij timvandermeij deleted the find-integration-test branch April 11, 2024 12:10
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.

Fix integration test "find bar highlight all must highlight text in the right position"
3 participants