Open ggardet opened 4 years ago
Ghostkeeper
commented
4 years ago I'm unable to reproduce this. The tests succeed for me, apart from the typing check which fails on Trust.py for me locally (as I have a more modern version of Mypy than our CI server).
The line in question is app.exec() on a QCoreApplication:
So that means that somewhere Qt is getting a segfault. That should never happen. We may need to bump this up to Qt or Riverbank.
gferon
commented
4 years ago I'm trying to package python-uranium for cura 4.5 and 4.6 for Fedora, and I'm getting the same segfault. I'm currently trying to investigate but came to the same conclusion as you did @Ghostkeeper (that it should never happen).
EDIT: what's suspicious is the test pass when executed one by one, which makes me think somewhere there's global state that makes them failing when running the entire suite.
Ghostkeeper
commented
4 years ago Maybe something in PyQt then? It looks like it creates multiple QCoreApplication instances within the same run of Python now (due to other tests creating one too). Maybe they don't properly get destroyed afterwards, or they indeed alter some global state in Qt somewhere?
You could try to move that QCoreApplication to a singleton, maybe, but the tests call the function quit() on it too so that could spell trouble.
StefanBruens
commented
4 years ago I would never expect to be able to create and destroy QCoreApplication multiple times. One QCoreApplication seems the much better option, maybe with a QEventLoop for each test if multiple event loops are deemed necessary.
Ghostkeeper
commented
4 years ago The application is re-created for every test in order to prevent previous tests from influencing subsequent tests. If they did, that would create some particularly nasty bugs in the tests that are hard to track down. Those bugs would also be exclusive to the tests, so fixing them is not making the application any better.
For me though and our CI server, this destroying and re-creating works fine. The Qt framework should allow that sort of thing.
StefanBruens
commented
4 years ago The application is re-created for every test in order to prevent previous tests from influencing subsequent tests.
Then you should also restart the python runtime, because it may contain some leftover state. Recreating the HttpRequestsManager should be completely sufficient.
If they did, that would create some particularly nasty bugs in the tests that are hard to track down. Those bugs would also be exclusive to the tests, so fixing them is not making the application any better.
You do not recreate the QApplication in the real application, so why do you do in the tests? If deleting the application were a requirement to make requests independent, then the tests would succeed, but the same requests in the real application would fail because there is some leftover state.
For me though and our CI server, this destroying and re-creating works fine. The Qt framework should allow that sort of thing.
Which version of Qt are you using? Have you tested Qt 5.12 and Qt 5.15, which are the current LTS releases? (And Qt 5.15 will be the last Qt5 version ever.)
Ghostkeeper
commented
4 years ago Yeah, we should restart the Python runtime, but that's not how PyTest works. Understandably, most unit tests wouldn't need this sort of thing and they'd claim that tests are expected to clean up after themselves.
Which version of Qt are you using?
We're currently on Qt 5.10.2. This is necessary to support some old MacOS versions that we still have a lot of users on. As that is dwindling though, we have a mind to upgrade to 5.12, which would solve a few open bugs (in particular a nasty one that crashes Cura if people use different GPUs for different screens).
StefanBruens
commented
4 years ago So, how about:
I could spend a little bit of time on this, iff this is deemed a sensible route forward and able to be merged.
Ghostkeeper
commented
4 years ago Yeah that is probably the only way to fix this.
The difficulty is probably in the 2nd point there. The HttpRequestManager in particular uses multithreading so you could end up with nondeterministic tests pretty quickly.
With Uranium 4.5.0, I get the following test failure: