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

Flaky test: Tests/test_file_eps.py::test_timeout #6877

Closed
hugovk opened this issue Jan 9, 2023 · 4 comments · Fixed by #6897
Closed

Flaky test: Tests/test_file_eps.py::test_timeout #6877

hugovk opened this issue Jan 9, 2023 · 4 comments · Fixed by #6897
Labels

Comments

@hugovk
Copy link
Member

hugovk commented Jan 9, 2023

test_timeout[Tests/images/timeout-d675703545fee17acab56e5fec644c19979175de.eps] in test_file_eps.py is a flaky test that intermittently fails. It often passes when restarted.

It often fails on the ubuntu-22.04-jammy-ppc64le and ubuntu-22.04-jammy-s390x Docker images jobs, which are pretty slow anyway - they take between 15-25 mins, so restarting can mean a long additional wait.

It would be good to fix this: fix/rewrite the test, or the code if the problem is there, or maybe even remove it. There are also pytest plugins to re-run flaky tests a number of times.

Perhaps this test should be skipped for these particular flavours.

The failure

=================================== FAILURES ===================================
_ test_timeout[Tests/images/timeout-d675703545fee17acab56e5fec644c19979175de.eps] _

test_file = 'Tests/images/timeout-d675703545fee17acab56e5fec644c19979175de.eps'

    @pytest.mark.timeout(timeout=5)
    @pytest.mark.parametrize(
        "test_file",
        ["Tests/images/timeout-d675703545fee17acab56e5fec644c19979175de.eps"],
    )
    def test_timeout(test_file):
        with open(test_file, "rb") as f:
            with pytest.raises(Image.UnidentifiedImageError):
>               with Image.open(f):

Tests/test_file_eps.py:294: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/vpy3/lib/python3.10/site-packages/PIL/Image.py:3268: in open
    im = _open_core(fp, filename, prefix, formats)
/vpy3/lib/python3.10/site-packages/PIL/Image.py:3254: in _open_core
    im = factory(fp, filename)
/vpy3/lib/python3.10/site-packages/PIL/ImageFile.py:117: in __init__
    self._open()
/vpy3/lib/python3.10/site-packages/PIL/EpsImagePlugin.py:227: in _open
    s_raw = fp.readline()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <PIL.EpsImagePlugin.PSFile object at 0x40086451e0>

    def readline(self):
        s = [self.char or b""]
        self.char = None
    
        c = self.fp.read(1)
>       while (c not in b"\r\n") and len(c):
E       Failed: Timeout >5.0s

/vpy3/lib/python3.10/site-packages/PIL/EpsImagePlugin.py:180: Failed

The logs

The test

@pytest.mark.timeout(timeout=5)
@pytest.mark.parametrize(
"test_file",
["Tests/images/timeout-d675703545fee17acab56e5fec644c19979175de.eps"],
)
def test_timeout(test_file):
with open(test_file, "rb") as f:
with pytest.raises(Image.UnidentifiedImageError):
with Image.open(f):
pass

See also

@hugovk hugovk added the Testing label Jan 9, 2023
@Yay295
Copy link
Contributor

Yay295 commented Jan 9, 2023

I think the PSFile class in EpsImagePlugin.py could be merged into EpsImageFile._open() (it's only usage) for some improved performance. Does PSFile need to exist for any backwards-compatibility reasons?

@hugovk
Copy link
Member Author

hugovk commented Jan 10, 2023

Yes, PSFile is part of the public API, so we'd need to raise deprecation warnings for at least a year first before it can be removed in a major bump, which would be Pillow 11.0.0 in October 2024.

https://github.com/python-pillow/Pillow/milestones

@radarhere
Copy link
Member

I've created PR #6897 as a suggestion for this.

@radarhere
Copy link
Member

#6897 was later reverted by #6879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants