search

rm-hull / luma.lcd

Python module to drive PCD8544, HT1621, ST7735, ST7567 and UC1701X-based LCDs
https://luma-lcd.readthedocs.io
MIT License
158 stars 57 forks source link

tests: add timeout using pytest-timeout #123

Closed thijstriemstra closed 3 years ago

thijstriemstra commented 3 years ago

fixes #122

thijstriemstra commented 3 years ago

@rm-hull is there a script to re-generate the json files that are causing the tests to fail? How do you regenerate them? Would be good to document this and/or add a script? I would like to do it in a separate PR so can you force merge this pull request (travis will fail regardless of this timeout change)?

rm-hull commented 3 years ago

As expected, if I pin pillow to a version prior to 8, then the tests pass. I just added commit 2a764a7 to verify this is the cause, and will revert it from this PR.

Rather than doing that here, I think we should pin pillow in luma.core instead. WDYT?

Interestingly, the py35 build succeeds because pillow never released version 8 (presumably because py35 is sunsetted), so it is using pillow 7.2 (see https://travis-ci.org/github/rm-hull/luma.lcd/jobs/736531130#L552)

thijstriemstra commented 3 years ago

Pillow 8 is py36 or newer only, they dropped 3.5 support in Pillow 8. Pillow 8 generates better results (e.g. that oval shape we saw in all the test images I updated) so I would definitely not want to pin pillow to something < 8. All we need to do is update the json files for pillow 8 and the tests will pass. Why would you want to pin it to older versions?

This is why I asked:

is there a script to re-generate the json files that are causing the tests to fail? How do you regenerate them? Would be good to document this and/or add a script? I would like to do it in a separate PR so can you force merge this pull request (travis will fail regardless of this timeout change)?

So I'd rather merge this pull request and work on updating the json files in a different pull request. You approved this PR but after you revert the change it'll fail again and you'll have to force merge this PR (I think).

rm-hull commented 3 years ago

Um .. im confused .. I thought the tests were failing because it hung travis for 10mins before being killed, and this pr was just about making it fail earlier. is it just the pytest diff algo that is making it hang rather than pillow?

rm-hull commented 3 years ago

All we need to do is update the json files for pillow 8 and the tests will pass.

Ok, that was not clear to me 😊

thijstriemstra commented 3 years ago

is it just the pytest diff algo that is making it hang rather than pillow?

I don't know what made it hang; but adding a timeout shows the actual errors: the json files are outdated.

rm-hull commented 3 years ago

i'll close the other PR in core ... as to regenerating JSON files, I think I ran something like tox -e py38 > temp.txt (with -vv in the tox.ini) and then for each failing test, pulled out the expected and reformatted into JSON. i.e. quite an arduous (and painful) manual task

I'll have a look to see if we can add a helper to make it more straightforward

thijstriemstra commented 3 years ago

i.e. quite an arduous (and painful) manual task

Yea! I see.

thijstriemstra commented 3 years ago

Pillow 8 generates better results (e.g. that oval shape we saw in all the test images I updated) so I would definitely not want to pin pillow to something < 8.

I wouldn't mind pinning Pillow to a minimum of 8.0 because it's much better.