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

Add basic GUI tests #72

Merged
merged 3 commits into from
Jun 6, 2023
Merged

Add basic GUI tests #72

merged 3 commits into from
Jun 6, 2023

Conversation

mfisher87
Copy link
Collaborator

@mfisher87 mfisher87 commented Jun 4, 2023 •
edited
Loading

Two new tests running in virtual framebuffer, made surprisingly easy by pytest-xvfb ❤️ :

EDIT: Resolves #71

@mfisher87 mfisher87 force-pushed the add-gui-tests branch 5 times, most recently from 4e3ed12 to d76fb1a Compare June 4, 2023 23:56
@mfisher87
Copy link
Collaborator Author

OK, learned something about #71. This was occurring for me because I was using Python 3.11. Python 3.9 test which reproduces this error XPASS, while 3.10 XFAIL. We get these deprecation warnings in the 3.9 tests:

 test/test_gui.py::TestGui::test_gui_edit_pyfile_opens
  /home/runner/work/viscm/viscm/viscm/gui.py:1157: DeprecationWarning: an integer is required (got type float).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.
    self.max_slider.setValue(viscm_editor.max_Jp)

test/test_gui.py::TestGui::test_gui_edit_pyfile_opens
  /home/runner/work/viscm/viscm/viscm/gui.py:1167: DeprecationWarning: an integer is required (got type float).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.
    self.min_slider.setValue(viscm_editor.min_Jp)

@mfisher87 mfisher87 force-pushed the add-gui-tests branch 4 times, most recently from 2e4951f to 1eb6e4f Compare June 5, 2023 00:47
@mfisher87
Fix failing viscm editor test
52758b9 
The old behavior was an implicit conversion to `int`. This warning was
being thrown in Python 3.9:

```
DeprecationWarning: an integer is required (got type float).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.
```

And in Python 3.10, this turned in to an error, as seen in
<matplotlib#71>:

```
TypeError: setValue(self, a0: int): argument 1 has unexpected type 'float'
```

The new version of this code simply makes the old behavior (conversion
to int) explicit. I'm unsure whether this was the original intent.
@mfisher87
Copy link
Collaborator Author

See the commit message of the most recent commit for details about the code change to gui.py!

@mfisher87 mfisher87 marked this pull request as ready for review June 5, 2023 00:52
@mfisher87 mfisher87 requested a review from stefanv June 5, 2023 00:52
@mfisher87 mfisher87 mentioned this pull request Jun 5, 2023
Merged
mfisher87
mfisher87 commented Jun 5, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
stefanv
stefanv approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me; I left two comments that you can choose whether to address or not.

doc/contributing.md
make test
```

Unit tests require `xvfb` (X Virtual Framebuffer) to test the GUI. If `xvfb` is not
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we skip the tests instead with a warning? That's what we do on most other projects with the pytest.importorskip decorator (reads: import or skip)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this will help, or I'm not quite understanding the suggestion :) xvfb isn't something we can import, it's a system dependency that needs to be present. I'll look in to other ways we might make this test conditional!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took some tinkering, but I figured out a good way to skip those tests: b86bf1a

viscm/cli.py

mainwindow.show()

# PyQt messes up signal handling by default. Python signal handlers (e.g.,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct that PyQt messes up signal handling? All it does is block the thread?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea on this one -- I just kept this part as it was when I refactored the _make_window function out!

@mfisher87 mfisher87 merged commit 7f55929 into matplotlib:main Jun 6, 2023
@mfisher87 mfisher87 deleted the add-gui-tests branch June 6, 2023 03:03
@mfisher87 mfisher87 mentioned this pull request Jun 6, 2023
Merged
@mfisher87 mfisher87 added this to the v0.10 milestone Jun 11, 2023
@mfisher87 mfisher87 mentioned this pull request Jun 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanv stefanv stefanv approved these changes

Assignees
No one assigned
Labels
bug ci
Projects
None yet
Milestone
v0.10
Development

Successfully merging this pull request may close these issues.

Error loading viridis from example_d.py
2 participants
@mfisher87 @stefanv