[MAINT] Reactivate GUI tests from the CI#3867
Conversation
…lled and similar API changes for PySide6
…ng if it has a size
…ting functionality
…creating the slicer to ensure an empty dictionary is not returned
|
@krzywon I wasn't able to reproduce the segfault locally on Ubuntu. I started fixing some other things, but I can add those in a separate PR if you are successful in making the pipeline pass 🤞 |
|
@backmari - I figured out the segfault issue (thanks to a not-be-named AI agent). I had to set to the Currently, the CI 'passes', but there are still a few segfaults that are being swallowed during the GUI tests. Once those are fixed, feel free to see if your changes conflict with what I've done and feel free to push here, if you can. |
|
Okay, @backmari. I think the GUI tests should be ready. Assuming this last CI passes, feel free to add your changes here. |
…and don't save a config file
|
@krzywon These were just some minor changes, probably nothing that will help with the segfaults (and hopefully won't break anything.) |
I had the same issue and couldn't figure out where it was coming from. Thank you! |
DrPaulSharp
left a comment
There was a problem hiding this comment.
This all looks good, it'll be great to get these tests back up and running. Alongside the specific comments:
- The command suggested in the PR does not work properly - specifically the
-p no:loggingdisables thecaplogfixture which causes errors in theguiUtilstests (alongside other tests skipped due to the use ofcaplog). Using--show-capture=noinstead should have the required effect without disablingcaplog - Using the command
python -m pytest --disable-warnings --show-capture=no .\src\sas\qtgui\on my Windows machine, I find:367 passed, 3 skipped, 29 xfailed, 129 warnings in 146.50s (0:02:26) - We should look into a way of closing dialogs generated by the test suite at a test or module level. They are correctly closed at the end of the session, but hang around for a while and may persist if an error occurs in the test suite.
- We need to work out the sequencing of this PR and PRs #3881 and #3888.
src/sas/qtgui/Calculators/UnitTesting/GenericScatteringCalculatorTest.py
Show resolved
Hide resolved
src/sas/qtgui/Calculators/UnitTesting/GenericScatteringCalculatorTest.py
Show resolved
Hide resolved
src/sas/qtgui/Calculators/UnitTesting/GenericScatteringCalculatorTest.py
Show resolved
Hide resolved
src/sas/qtgui/Calculators/UnitTesting/GenericScatteringCalculatorTest.py
Outdated
Show resolved
Hide resolved
src/sas/qtgui/Perspectives/Inversion/UnitTesting/InversionPerspectiveTest.py
Outdated
Show resolved
Hide resolved
|
@DrPaulSharp, thanks for the review. I'll respond to your comments soon, but when I was working on this, I prioritized getting things working and passing, not understanding whey I had to change the tests. I'll work on the proposed changes soon. As far as merge order, I think this one -> #3888 -> #3881 would be best. Each PR should be rebased to main after others have been merged to be sure the unit tests continue to pass. |
This all makes sense. @jellybean2004 are you happy with the proposed merge order and process here? |
|
My latest test results, using
|
DrPaulSharp
left a comment
There was a problem hiding this comment.
Ok, I'm still getting a lot of warnings after upgrading periodictable: 366 passed, 3 skipped, 30 xfailed, 129 warnings in 164.43s (0:02:44) but that aside, I think we can merge.
We need to raise an issue on the graphs and dialogs that are kept open throughout the test suite.
|
IMPORTANT - lines 203-207 of otherwise running pytest without specifying a directory tries to run tests in Do we want to run the GUI tests in the CI as well? At present we only run the tests in the |
That's the point of this PR. Our CI now runs against I'm planning to merge this soon and will add two issues once I've done that:
|
| # The test-installer job can run with the pyinstaller debug bundle | ||
| INSTALLER_USE_DEBUG: false | ||
| # https://pytest-qt.readthedocs.io/en/latest/troubleshooting.html - Required for PySide pytest runner | ||
| DISPLAY: ':99.0' |
There was a problem hiding this comment.
@krzywon just noticed this now (sorry) - setting DISPLAY like this here but allowing xvfb-run to pick its own display number with -a is fragile. -a is for automatically determining which display to use, starting at :99. It also should be unnecessary as xvfb-run sets DISPLAY on its own and sets it to the value of the display that it has created. It should be safe(r) to remove this when next touching the code.
There was a problem hiding this comment.
I'll open an issue to remove it. This was something I tried after reading the commented link, but wasn't sure it was doing anything, and based on what you're suggesting, it likely isn't.
Description
This is currently a draft because I don't know if linux will handle the tests as well as Windows.
This is a comprehensive redo of the GUI tests. This includes a number of changes to fix a series of the tests, but I couldn't get them all working. Running the tests locally, I got pretty far, but need to stop where I am and move on to other things. Currently, 38 tests are labelled as skip or xfail, and four test files are not run (more below).
Things changed
xfailandskipAddMultiEditorTest.pyto a sub folder ofUtilites\UnitTestingand added that directory to the pytestnorecursedirslist (Loading the editor is segfaulting right now...)norecursedirslist because of the major changes to it - most all of the tests currently failnorecursedirslist because it is still experimental and many of its tests failFixes #2322
How Has This Been Tested?
Ran
python -m pytest --disable-warnings -p no:logging .\src\sas\qtgui\from the base sasview directory.Review Checklist:
[Documentation (check at least one)
Installers
Licensing (untick if necessary)