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

Fix Tribler startup/shutdown and GUI tests stability #6828

Merged

Conversation

kozlovsky
Copy link
Contributor

@kozlovsky kozlovsky commented Mar 23, 2022

Fixes #6583.

It seems I was able to fix unstable GUI tests. I tested this PR 15 times in a row without a single failure, while without this PR, almost every run of GUI tests ends with a failure.

The PR consists of several mostly independent commits; it is easier to review them on a per-commit basis.

The first commit changes the logic of the QApplication shutdown to prevent RuntimeError exceptions "wrapped C/C++ object of type ... has been deleted". The reason for the error is that we should not touch any Qt object after the QApplication.quit() invocation, as Qt objectы can already be partially destroyed at this stage. It is not easy to satisfy this requirement, as some Qt signals are emitted after QApplication.quit() was already called, for example QProcess.readyReadStandardOutput and QProcess.readyReadStandardError. Also, if an exception hook catches some exception during the application shutdown, it should not open FeedbackDialog as it can already be destroyed as well.

QCoreApplication has a special closingDown property that was apparently added for this type of check. Unfortunately, it is not working correctly for some reason - it returns False inside a signal during the application closing down, and any attempt to access a Qt object immediately after that leads to the same RuntimeError "wrapped C/C++ object ... has been deleted".

To solve this, I added a special AppManager class that handles the application shutdown. Now, instead of directly calling QApplication.quit(), you should always call app_manager.quit_application(). A code can check the app_manager.quitting_app flag before accessing a Qt object to ensure that the application is not shutting down. This way, all runtime errors "wrapped C/C++ object ... has been deleted" were eliminated.

The reason for implementing a separate AppManager class instead of adding the functionality into an existing class (like TriblerWindow) is to avoid circular imports and to simplify dependency injections.

The second commit renames a signal EventRequestManager.tribler_started to core_connected to better reflect the actual purpose of the signal - it is emitted at the moment GUI starts receiving events from Core.

Then I rewrote the wait_for_signal function used in GUI tests. It turns out the previous implementation was completely broken - it used a global variable signal_received that was not reset after the usage. In GUI tests, the wait_for_signal function was called multiple times, first inside the window fixture to wait for the application to start and then in specific tests to wait for a particular GUI state. As a result, wait_for_signal never worked properly inside tests, as the signal_received flag was already set in the window fixture and newer reset.

The main fix for the wait_for_signal function is in passing to the function a dynamically calculated condition that is re-evaluated on each iteration instead of passing just a static value of a boolean flag.

After I fixed the wait_for_signal function, I discovered a broken test_tags_dialog test and fixed it as well.

@kozlovsky
Copy link
Contributor Author

retest this please

3 similar comments
@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky kozlovsky force-pushed the fix/tribler_start_shutdown_gui_tests branch from 4bf095f to 0ca3212 Compare March 23, 2022 04:19
@kozlovsky
Copy link
Contributor Author

retest this please

4 similar comments
@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky kozlovsky force-pushed the fix/tribler_start_shutdown_gui_tests branch 3 times, most recently from e7be306 to 315439a Compare March 23, 2022 06:45
@kozlovsky kozlovsky marked this pull request as ready for review March 23, 2022 06:57
@kozlovsky kozlovsky requested review from a team, xoriole, drew2a and devos50 and removed request for a team March 23, 2022 06:57
@kozlovsky kozlovsky changed the title [WIP] Fix Tribler startup/shutdown and GUI tests stability Fix Tribler startup/shutdown and GUI tests stability Mar 23, 2022
drew2a
drew2a previously approved these changes Mar 23, 2022
@kozlovsky kozlovsky force-pushed the fix/tribler_start_shutdown_gui_tests branch from 315439a to b5349c6 Compare March 23, 2022 13:39
@kozlovsky
Copy link
Contributor Author

GUI tests are passing, but core tests are still sometimes aborted on timeout. For example, right now Core tests was timed out on test_add_content_dir. The test looks absolutely innocent and does not have anything that can block:

def test_add_content_dir(tdef):
    """
    Test whether adding a single content directory with two files is working correctly.
    """
    torrent_dir = TESTS_DATA_DIR / "contentdir"
    tdef.add_content(torrent_dir / "file.txt")
    tdef.add_content(torrent_dir / "otherfile.txt")
    tdef.save()

    metainfo = tdef.get_metainfo()
    assert len(metainfo[b'info'][b'files']) == 2

It seems that for core tests, the deadlocks are caused not by tests themselves but by something in the pytests setup.

Anyway, GUI tests stability seems to be fixed.

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky
Copy link
Contributor Author

retest this please

1 similar comment
@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky kozlovsky force-pushed the fix/tribler_start_shutdown_gui_tests branch from b5349c6 to da025dc Compare March 23, 2022 15:13
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky
Copy link
Contributor Author

GitHub web hooks and actions status is degraded today: https://www.githubstatus.com/

@kozlovsky
Copy link
Contributor Author

retest this please

1 similar comment
@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky kozlovsky merged commit fccf4fa into Tribler:main Mar 23, 2022
@kozlovsky kozlovsky deleted the fix/tribler_start_shutdown_gui_tests branch March 23, 2022 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants