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

Use PDM & Ruff instead of Poetry+Black+Flake8+isort #82

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

justinmayer
Copy link
Contributor

This also pins Pillow to 9.x, since I observed four test failures when running tests on 10.x:

========================================================================== short test summary info ==========================================================================
FAILED pelican/plugins/image_process/test_image_process.py::test_all_transforms[image_path0-detail-transform_params10] - assert (320, 31, 792, 721) is None
FAILED pelican/plugins/image_process/test_image_process.py::test_all_transforms[image_path0-smooth_more-transform_params16] - assert (16, 0, 1016, 728) is None
FAILED pelican/plugins/image_process/test_image_process.py::test_all_transforms[image_path1-detail-transform_params10] - assert (319, 65, 740, 716) is None
FAILED pelican/plugins/image_process/test_image_process.py::test_all_transforms[image_path1-smooth_more-transform_params16] - assert (327, 72, 741, 706) is None

@MinchinWeb
Copy link
Member

I'm not crazy about pinning Pillow back, as most upgrades there seem to be to fix a CVE.

Does this change the testing framework? (I noticed a bunch of pytest plugins look like they're no longer being installed) These failing tests aren't new because of that, are they?

@justinmayer
Copy link
Contributor Author

justinmayer commented Jul 7, 2024

I also prefer to avoid long-term pins. Think of this pin as a temporary measure until the issue with the failing tests can be sorted out.

Testing framework is unchanged. Which Pytest plugins do you think are no longer being installed? While I modified the linters, as far as I can tell, the list of Pytest-related dependencies has not changed.

I had already confirmed that the same tests fail in current main, without any of the changes in this PR. I decided to address that here via the aforementioned pin to avoid test failures in this PR.

@MinchinWeb
Copy link
Member

MinchinWeb commented Jul 8, 2024

I also prefer to avoid long-term pins. Think of this pin as a temporary measure until the issue with the failing tests can be sorted out.

Ok, let's create an issue for it, so it doesn't get forgotten about.

One issue I've run into previously is that the tests will fail if the generated image from Pillow isn't a bit-perfect match of the previous results. Would you run the "regenerate image" command, check to make sure they visually match, and then re-run the tests with new images?

This doesn't have to hold up this PR, but might be the simplest way to solve these "failing" tests. The code is thus (you'll need Pillow v10 installed; pulled from the Readme):

>>> from pelican.plugins.image_process.test_image_process import generate_test_images
>>> generate_test_images()
36 test images generated!

Testing framework is unchanged. Which Pytest plugins do you think are no longer being installed?

Disregard. I was reviewing it on my phone this morning, and the diff view was unclear. Now on the computer, it's easy to see it's staying the same.

Otherwise, it looks good. Feel free to merge!

@justinmayer
Copy link
Contributor Author

justinmayer commented Jul 8, 2024

I followed the provided instructions, but running generate_test_images() did not produce any difference in the resulting image files that I could detect.

I did a few experiments and tracked the problem down to Pillow 10.3.0, which introduced some change that causes those four tests to fail — a problem that is also present with the current Pillow 10.4.0. So I adjusted the pin to expand the range to: >9.1,<10.3

I'll merge this for now. See below for a link to a new issue I created to track the problem with Pillow 10.3+.

@justinmayer justinmayer merged commit da11203 into main Jul 8, 2024
15 checks passed
@justinmayer justinmayer deleted the replace-poetry-with-pdm branch July 8, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants