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

Pillow 10.2.0 breaks kraken3 screen smoke test #661

Open
jonasmalacofilho opened this issue Jan 11, 2024 · 1 comment
Open

Pillow 10.2.0 breaks kraken3 screen smoke test #661

jonasmalacofilho opened this issue Jan 11, 2024 · 1 comment
Labels
bug Apparent bug in liquidctl

Comments

@jonasmalacofilho
Copy link
Member

jonasmalacofilho commented Jan 11, 2024

Describe the bug

Due to python-pillow/Pillow#7568, some local GIF headers are no longer omitted with pillow 10.2.0, and this breaks our very simple kraken3 screen smoke test:

 =========================== short test summary info ============================
FAILED tests/test_kraken3.py::test_krakenz3_not_totally_broken - AssertionError: Bulk write failed, wrong data for mode: gif, data index: 0
assert [18, 250, 1, ...171, 205, ...] == [18, 250, 1, ...171, 205, ...]
  At index 16 diff: 205 != 189
  Full diff:
  - [18, 250, 1, 232, 171, 205, 239, 152, 118, 84, 50, 16, 1, 0, 0, 0, 189, 6, 0, 0]
  ?                                                                    ^^^
  + [18, 250, 1, 232, 171, 205, 239, 152, 118, 84, 50, 16, 1, 0, 0, 0, 205, 6, 0, 0]
  ?                                                                    ^^^
=================== 1 failed, 435 passed, 1 warning in 4.64s ===================
Error: Process completed with exit code 1.

We probably want our test to be more tolerant of innocuous differences in the prepared GIF, instead of just changing the hardcoded expected value to match the new pillow version (and only that version). At the same time, we probably also want to keep things simple.

I tried substituting the equality comparisons with minimum difflib.SequenceMatcher.ratio()s between the actual bulk writes and our golden samples. But, while that worked, it's a blunt approach, and one that would easily allow future bugs to go undetected. Since we only have that one smoke test for everything related to the kraken3 screen, I don't thing it's wise to make it even less robust.

Any ideias for a way to tackle this that doesn't involve rewriting the entire mock and test, as well as duplicating some of the logic (e.g. write splitting) from the driver in the test itself?


Fixes: 65d7e75
Fixes: #659

Presumably this does not affect real devices.

Commands executed

$ python -m venv venv
$ venv/bin/pip install .
$ venv/bin/pip install pytest
$ venv/bin/python -m pytest -k kraken3 -v

Output of all relevant commands with --debug flag

Affected device

No response

Does your version of liquidctl support the device in question?

Yes, my version supports it

Operating system and version

All

Installation method

From sources

Version of liquidctl

65d7e75

@jonasmalacofilho jonasmalacofilho added the bug Apparent bug in liquidctl label Jan 11, 2024
@aleksamagicka
Copy link
Member

Setting GIFs works OK with the new version. Has this ever happened before? The upstream change looks like an overall win, so if it's not too frequent for the mean time the test could just be updated for the new output (and stop breaking CI). I've got no better idea currently.

evils added a commit to evils/nixpkgs that referenced this issue Feb 11, 2024
evils added a commit to evils/nixpkgs that referenced this issue Feb 11, 2024
evils added a commit to evils/nixpkgs that referenced this issue Feb 11, 2024
evils added a commit to evils/nixpkgs that referenced this issue Feb 11, 2024
issue described upstream in
  liquidctl/liquidctl#661

manually backported for 23.11 where there's still
  SETUPTOOLS_SCM_PRETEND_VERSION
    which was removed as redundant on master shortly after 23.11
evils added a commit to evils/nixpkgs that referenced this issue Feb 11, 2024
issue described upstream in
  liquidctl/liquidctl#661

manually backported for 23.11 where there's still
  SETUPTOOLS_SCM_PRETEND_VERSION
    which was removed as redundant on master shortly after 23.11
Kranzes pushed a commit to NixOS/nixpkgs that referenced this issue Feb 12, 2024
* liquidctl: fix test for pillow ≥ 10.2.0

issue described upstream in
  liquidctl/liquidctl#661
Emantor pushed a commit to Emantor/nixpkgs that referenced this issue Feb 13, 2024
* liquidctl: fix test for pillow ≥ 10.2.0

issue described upstream in
  liquidctl/liquidctl#661
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Apparent bug in liquidctl
Projects
None yet
Development

No branches or pull requests

2 participants