Skip to content

Conversation

Nid01
Copy link

@Nid01 Nid01 commented Oct 5, 2025

Partly undo d89fb78#diff-153e5cee71ddf471246ff5c0e198345f005942a540a8f39297736f0b620d631dR1587-R1592 since the changed order of updates to the page boundaries results in wrongly scaled pdfs.

@Nid01 Nid01 marked this pull request as ready for review October 5, 2025 21:29
@Nid01
Copy link
Author

Nid01 commented Oct 5, 2025

I'm not sure if I'm required to create a test for this fix.
The commit which caused the issue didn't have one, so I also don't need one?
In case I need one I don't really know how to create such test since I'm not familiar with in dept programming and appreciate every help to get the issue fixed.

Copy link

codecov bot commented Oct 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.09%. Comparing base (9fad9ff) to head (fc01c9a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3489   +/-   ##
=======================================
  Coverage   97.09%   97.09%           
=======================================
  Files          56       56           
  Lines        9658     9658           
  Branches     1748     1748           
=======================================
  Hits         9377     9377           
  Misses        168      168           
  Partials      113      113           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stefan6419846
Copy link
Collaborator

I'm not sure if I'm required to create a test for this fix.
The commit which caused the issue didn't have one, so I also don't need one?

I have not been aware that the faulty commit would not have proper tests, thus I did not require a new one. With the current regression, we should add one to ensure this does not break again.

In case I need one I don't really know how to create such test since I'm not familiar with in dept programming and appreciate every help to get the issue fixed.

This is a very strange side effect, as the boxes all look correct:

@pytest.mark.enable_socket
def test_scale_by():
    """Tests for #3487"""
    url = "https://github.com/user-attachments/files/22685841/input.pdf"
    name = "issue3487.pdf"
    reader = PdfReader(BytesIO(get_data_from_url(url, name=name)))

    original_box = RectangleObject((0, 0, 595.275604, 841.88974))
    expected_box = RectangleObject((0.0, 0.0, 297.637802, 420.94487))
    for page in reader.pages:
        assert page.artbox == original_box
        assert page.bleedbox == original_box
        assert page.cropbox == original_box
        assert page.mediabox == original_box
        assert page.trimbox == original_box

        page.scale_by(0.5)
        assert page.artbox == expected_box
        assert page.bleedbox == expected_box
        assert page.cropbox == expected_box
        assert page.mediabox == expected_box
        assert page.trimbox == expected_box

Thus, we need a rendering test with Ghostscript here to actually detect this bug (which has already been fixed in the past in #1314 - without adding a test).

@stefan6419846 stefan6419846 added the needs-test A test should be added before this PR is merged. label Oct 7, 2025
@stefan6419846
Copy link
Collaborator

If I am not mistaken, the added test is not sufficient, as running it against main is successful as well, not providing any benefits (as outlined in my comment as well). We have to add a test similar to the discussions in the issue by comparing the rendering results with Ghostscript, as mentioned in my comment as well (examples are already available for watermarking for example).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-test A test should be added before this PR is merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants