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.

Sign up for GitHub

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

Jump to bottom

Test failure in test_assert_bookmarks with Python 3.12 #1902

Closed
FelixSchwarz opened this issue Jul 6, 2023 · 6 comments
Closed

Test failure in test_assert_bookmarks with Python 3.12 #1902

FelixSchwarz opened this issue Jul 6, 2023 · 6 comments
Labels
bug Existing features not working as expected

Comments

@FelixSchwarz
Copy link
Contributor

Fedora's Python wranglers just completed the Python 3.12 mass rebuild and unfortunately there is a WeasyPrint test failing:

=================================== FAILURES ===================================
_ test_assert_bookmarks[\n        <body>\n        <h1 style="width: 10px; line-height: 10px;\n                   transform: skew(45deg, 45deg)">!\n    -expected_by_page9-expected_tree9-False] _

html = '\n        <body>\n        <h1 style="width: 10px; line-height: 10px;\n                   transform: skew(45deg, 45deg)">!\n    '
...
E       AssertionError: assert [[(1, '!', (-...99), 'open')]] == [[(1, '!', (-5, -5), 'open')]]
E         At index 0 diff: [(1, '!', (-4.999999999999999, -4.999999999999999), 'open')] != [(1, '!', (-5, -5), 'open')]
E         Use -v to get more diff
=============================== warnings summary ===============================
...
=========================== short test summary info ============================
FAILED tests/test_api.py::test_assert_bookmarks[\n        <body>\n        <h1 style="width: 10px; line-height: 10px;\n                   transform: skew(45deg, 45deg)">!\n    -expected_by_page9-expected_tree9-False]

python3-cffi              1.15.1-5.fc39
python3-cssselect2        0.7.0-3.fc39 
python3-fonttools         4.40.0-2.fc39
python3-fonttools+woff    4.40.0-2.fc39
python3-html5lib          1:1.1-12.fc39
python3-pillow            9.5.0-2.fc39 
python3-pydyf             0.6.0-3.fc39 
python3-pyphen            0.13.2-3.fc39
python3-tinycss2          1.2.1-4.fc39

The problem is reproducible in Fedora's containerized build infrastructure (mock). Should I try to debug this further or maybe you already know where this could be happening?
@liZe
Copy link
Member

liZe commented Jul 7, 2023

Hi!

Thanks for the report.

Maybe it’s caused by this improvement, that’s the only change related to floats I can find in Python 3.12’s changelog.

There’s actually no need to search where it really comes from, it’s not a real bug, just one of the many rounding approximation we have in WeasyPrint (and in all browsers, to be honest!). Changing this False to True should fix the problem. If it does, you can open an PR, or ask me to commit the change.

@FelixSchwarz
Copy link
Contributor Author

You are spot on with regard to round.

With regards to a PR I was wondering if just rounding is the right answer. I found a comment regarding self.bookmarks in Page:

    #: The :obj:`list` of ``(level, label, target, state)``
    #: :obj:`tuples <tuple>`. ``level`` and ``label`` are respectively an
    #: :obj:`int` and a :obj:`string <str>`, based on the CSS properties
    #: of the same names. ``target`` is an ``(x, y)`` point in CSS pixels
    #: from the top-left of the page.
    self.bookmarks = []

So I'm wondering if the right fix is to ensure that the tuple in page.bookmarks only contains an int?

@liZe
Copy link
Member

liZe commented Jul 9, 2023

You are spot on with regard to round.

👍

So I'm wondering if the right fix is to ensure that the tuple in page.bookmarks only contains an int?

The (x, y) tuple contains the target coordinates. All coordinates and sizes in WeasyPrint (and other browsers) are floats, because integers are not sufficient to get the granularity we want to position everything. For bookmarks, integers would probably be OK, but I think that 1) some users depend on very precise bookmark coordinates for some use case we can’t think about, and 2) there’s no need to have a special case for bookmarks just to make tests pass.

The real solution would be to find which operations cause this rounding error and find a way to avoid it. I suspect the transformation matrix multiplications caused by the skew, but I don’t know why it happens just with Python 3.12. By the way, as other transformations already introduced rounding errors, as we already have a workaround for this in tests, I think that we can safely use the workaround.

@liZe liZe added the bug Existing features not working as expected label Jul 9, 2023
@liZe liZe closed this as completed in 4043c1f Jul 9, 2023
@FelixSchwarz
Copy link
Contributor Author

Thank you - maybe we should also adapt the comment in the Page class as the coordinates are not (always) int? Or did I misread the comment?

@liZe
Copy link
Member

liZe commented Jul 9, 2023

level is an int, but (x, y) is a point in CSS pixels, and can thus contain floats. Is there a problem in the comment?

@FelixSchwarz
Copy link
Contributor Author

No, I just misread the comment. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Existing features not working as expected
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@FelixSchwarz @liZe