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.

Sign up for GitHub

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

Jump to bottom

Unix Viewers - 2nd attempt #6021

Closed
wants to merge 5 commits into from
Closed

Unix Viewers - 2nd attempt #6021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b348881
Viewer class properties: src/PIL/ImageShow.py, Tests/test_imageshow.p…
alv2017 Feb 3, 2022
450180f
UnixViewers modification: src/PIL/ImageShow.py, Tests/test_imageshow.py
alv2017 Feb 3, 2022
30227cd
Correction to Tests/test_imageshow.py::test_show
alv2017 Feb 7, 2022
6a84854
src/PIL/ImageShow.py: UnixViewers.show_file() method have been updated.
alv2017 Feb 8, 2022
1ec32b7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions Tests/helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@
class test_image_results:
@staticmethod
def upload(a, b):
a.show()
b.show()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is hit, the tests will hang infinitely. I do not see any good reason for a.show(), b.show(): we are interested in return value of the upload function, and return value here is None.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? This is only used if there is an environment variale SHOW_ERRORS set, which means the user is asking to be shown failed image comparisons. Removing this breaks that functionality. Also, the result of the upload function is irrelevant as there is a check in place to see if it is defined. If it isn't defined, it is never called anyway, so if this were to be removed, you would remove the whole if block.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. As I mentioned above, I implemented the viewers as a UI part, and it was not an option to use them in tests.

  2. To be honest, I find this part of code quite confusing:

Tests/helper.py

if os.environ.get("SHOW_ERRORS", None):
    # local img.show for errors.
    HAS_UPLOADER = True

    class test_image_results:
        @staticmethod
        def upload(a, b):
            return None

elif "GITHUB_ACTIONS" in os.environ:
    HAS_UPLOADER = True

    class test_image_results:
        @staticmethod
        def upload(a, b):
            dir_errors = os.path.join(os.path.dirname(__file__), "errors")
            os.makedirs(dir_errors, exist_ok=True)
            tmpdir = tempfile.mkdtemp(dir=dir_errors)
            a.save(os.path.join(tmpdir, "a.png"))
            b.save(os.path.join(tmpdir, "b.png"))
            return tmpdir

else:
    try:
        import test_image_results

        HAS_UPLOADER = True
    except ImportError:
        pass

return None

elif "GITHUB_ACTIONS" in os.environ:
HAS_UPLOADER = True
Expand Down Expand Up @@ -316,6 +315,10 @@ def is_win32():
return sys.platform.startswith("win32")


def is_macos():
return sys.platform.startswith("darwin")


def is_pypy():
return hasattr(sys, "pypy_translation_info")

Expand Down
51 changes: 39 additions & 12 deletions Tests/test_imageshow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from PIL import Image, ImageShow

from .helper import hopper, is_win32, on_ci
from .helper import hopper, is_macos, is_win32, on_ci


def test_sanity():
Expand Down Expand Up @@ -41,9 +41,10 @@ def show_image(self, image, **options):
ImageShow._viewers.pop(0)


@pytest.mark.skipif(
not on_ci() or is_win32(),
reason="Only run on CIs; hangs on Windows CIs",
@pytest.mark.skip(
reason="""Current implementation of some viewers requires manual closing of an image,
because of that the tests calling show() method will hang infinitely.
"""
)
def test_show():
for mode in ("1", "I;16", "LA", "RGB", "RGBA"):
Expand All @@ -54,7 +55,7 @@ def test_show():
def test_viewer():
viewer = ImageShow.Viewer()

assert viewer.get_format(None) is None
assert viewer.get_format(None) == "PNG"

with pytest.raises(NotImplementedError):
viewer.get_command(None)
Expand All @@ -63,9 +64,33 @@ def test_viewer():
def test_viewers():
for viewer in ImageShow._viewers:
try:
viewer.get_command("test.jpg")
cmd = viewer.get_command("test.jpg")
assert isinstance(cmd, str)
except NotImplementedError:
pass
assert isinstance(viewer, ImageShow.IPythonViewer)


@pytest.mark.skipif(
is_win32() or is_macos(), reason="The method is implemented for UnixViewers only"
)
def test_get_command_ex_interface():
"""get_command_ex() method used by UnixViewers only"""

file = "some_image.jpg"
assert isinstance(file, str)

for viewer in ImageShow._viewers:
if isinstance(viewer, ImageShow.UnixViewer):
# method returns tuple
result = viewer.get_command_ex(file)
assert isinstance(result, tuple)
# file name is a required argument
with pytest.raises(TypeError) as err:
viewer.get_command_ex()
assert (
"get_command_ex() missing 1 required positional argument: 'file'"
in str(err.value)
)


def test_ipythonviewer():
Expand All @@ -89,10 +114,12 @@ def test_file_deprecated(tmp_path):
f = str(tmp_path / "temp.jpg")
for viewer in ImageShow._viewers:
hopper().save(f)
with pytest.warns(DeprecationWarning):
try:
viewer.show_file(file=f)
except NotImplementedError:
pass
if not isinstance(viewer, ImageShow.UnixViewer):
# do not run this assertion with UnixViewers due to implementation
with pytest.warns(DeprecationWarning):
try:
viewer.show_file(file=f)
except NotImplementedError:
pass
with pytest.raises(TypeError):
viewer.show_file()