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

Run unit tests of Qt GUI and have CI gate on them #2252

Merged
merged 187 commits into from
Oct 25, 2022

Conversation

llimeht
Copy link
Contributor

@llimeht llimeht commented Oct 15, 2022

The unittests of the Qt GUI have been running (and failing) on CI for a long time. The failures have been suppressed because the size of the job to get these tests running cleanly was too big.

In this PR, we finally get something that looks like running unit tests. In getting the tests to work, we see that there are a few bugs from the recent config class work and we can fix them.

Following up on the discussion in #1732, the approach here has been to:

  1. Switch to pytest as a driver for the unittests.
  2. Mark individual tests as failing (xfail) and/or applying some obvious fixes to tests. Some tests cause the Python interpreter to fail with a segmentation fault so are skipped instead
  3. Convert unittest's assertFoo functions to normal assert calls to match pytest style
  4. Convert unittest setup/teardown code to pytest fixtures; in particular, converting the creation of widgets to the qapp fixture from pytest-qt. Using qapp gets away from lots of segmentation faults caused by incorrectly handling QApplication across tests and race conditions with garbage collection.
  5. Fixing the mocks to use pytest-mock; the previous liberal use of MagicMock modified the actual class definitions in many places and the modifications were not reversed after the tests, meaning that tests had side-effects and could cause each other to fail depending on the order in which the tests were run.
  6. Run through some still failing tests, look for some fixes to either the main code or to the tests.

The final result of this is not perfect, but I think it's now objectively better than the current situation. This branch is also now unmaintainably large, touching so many files that conflicts are becoming costly to resolve.

= 354 passed, 36 skipped, 54 xfailed, 6 xpassed, 26 warnings in 74.02s (0:01:14) =

After merging the following tasks remain outstanding:

  • fix file handling in tests; this will likely be a simple fixture in a conftest.py that uses importlib.resources to locate the test input files. Consolidation of the multiple copies of some of the files should happen first. This would fix 29 tests that are currently xfailed.
  • look systematically at individual tests that are xfailed; many of these will be simple to fix, either updating the UI code or updating the test. These can be done one at a time.
  • check the individual tests that are now "xpass" (i.e. expected to fail but are now passing); these are tests that were failing but through other changes (most likely the changes to the mocking) now pass.
  • likewise for the skipped tests; these are mostly failing due to causing the Python interpreter to segfault and so will need a careful review of the code and tests in how widgets are created and destroyed (some are not destroyed at all in the tests; some are destroyed in the wrong order)
  • understanding what is causing these segfaults will allow a bit of chasing through the other tests to fix the remaining causes of the segfaults

The first three of these are more bite-sized pieces of work and would be compatible with code-camp for instance, while this 158 commit behemoth would not be.

The last two items around segfaults are harder problems. It's hard to spot if these are bugs in the UI or bugs in the testing code at this stage.

(The commits from #2214 are included here as without them, the tests can't succeed)

Closes: #1732

This is probably not the correct long term solution, but lets all the tests in this set pass
which is a reasonable step forward
@llimeht
Copy link
Contributor Author

llimeht commented Oct 25, 2022

@krzywon, thanks for the review. All the items other than the windows-specific failure dealt with. Dealing with that one in a separate PR (that could then enable GUI testing on windows too!) would probably be reasonable.

I'm a little nervous about having this unmerged while codecamp is happening. Lack of tests is unhelpful for the codecampers, but we'll also end up tripping over each other with merge conflicts.

Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

The latest changes look great and should probably be merged sooner rather than later.

@butlerpd butlerpd merged commit aa3917d into SasView:main Oct 25, 2022
@llimeht llimeht deleted the tmp/guitests branch October 26, 2022 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit testing of qtgui
3 participants