[wdspec] browsingContext.print: fix rounding error in page.py test#40504
Merged
DanielRyanSmith merged 5 commits intomasterfrom Jun 16, 2023
Merged
[wdspec] browsingContext.print: fix rounding error in page.py test#40504DanielRyanSmith merged 5 commits intomasterfrom
DanielRyanSmith merged 5 commits intomasterfrom
Conversation
Member
Author
e58b369 to
5e2335a
Compare
Member
Author
|
I confirm this fixes all failing tests on our end: 
|
Member
Author
|
@foolip rounding is hard, eh? ;) |
whimboo
reviewed
Jun 12, 2023
[pytest](https://github.com/web-platform-tests/wpt/blob/7a087d54be8b6c0ca0181a86dc1ff0b28461c383/webdriver/tests/support/image.py) uses: def cm_to_px(cm): return round(cm * 96 / 2.54) [js](https://github.com/web-platform-tests/wpt/blob/7a087d54be8b6c0ca0181a86dc1ff0b28461c383/tools/wptrunner/wptrunner/print_pdf_runner.html) uses: const viewport = page.getViewport({ scale: 96. / 72. }); ... canvas.height = viewport.height; canvas.width = viewport.width; This produces a rounding error, even though the dimension is correct: > assert cm_to_px(expected_dimensions["height"]) == height E assert 454 == 453 E +454 E -453 The inconsistency of rounding in both ends becomes clear when we eliminate "round" in the pytest side: > assert cm_to_px(expected_dimensions["height"]) == height E assert 453.54330708661416 == 453 E +453.54330708661416 E -453 There are multiple ways to fix this issue. Option #1: Use "floor" instead of "round" in pytest. Option #2: Use a range in the assertion comparison, allowing a difference of up to +-1.0. This is what this PR does. The comparison is performed in [`assert_pdf_dimensions`](https://github.com/web-platform-tests/wpt/blob/b6107cc1ac8b9c2800b4c8e58af719b8e4d9b8db/webdriver/tests/support/fixtures_bidi.py#L210). The problematic part is .96 / .72 which evaluates to 4/3 = 1.333333....
5e2335a to
4ceecd6
Compare
4ceecd6 to
7e65d91
Compare
Member
Author
|
Here's another way that works @whimboo: In case you don't like the approach to compare to a +-1.0 range. |
whimboo
reviewed
Jun 14, 2023
added 2 commits
June 14, 2023 15:19
Member
Author
@jgraham could you merge this? Looking into the logs, it seems to me the failures are unrelated (but please double check). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pytest uses:
js uses:
This produces a rounding error, even though the dimension is correct:
The inconsistency of rounding in both ends becomes clear when we eliminate "round" in the pytest side:
There are multiple ways to fix this issue.
Option #1: Use "floor" instead of "round" in pytest. This approach fixes some tests, but others still fail. I guess in some cases "round" is correct.
Option #2: Use a range in the assertion comparison, allowing a difference of up to +-1.0. This is what this PR does.
The comparison is performed in
assert_pdf_dimensions.The problematic part is .96 / .72 which evaluates to 4/3 = 1.333333....