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 gui tests stability #6619

Merged
merged 6 commits into from
Dec 6, 2021
Merged

Conversation

kozlovsky
Copy link
Contributor

In this PR, I rewrote the logic of CoreManager to fix the shutdown process sequence and make the logic of CoreManager easier to understand.

With these changes, I was able to get completely stable GUI tests on my Windows machine. Previously I often got the following error (reported in #6039):

Traceback (most recent call last):
  File "C:\dev\tribler\src\tribler-gui\tribler_gui\utilities.py", line 375, in trackback_wrapper
    raise exc from CreationTraceback(traceback_str)
  File "C:\dev\tribler\src\tribler-gui\tribler_gui\utilities.py", line 372, in trackback_wrapper
    callback(*args, **kwargs)
  File "C:\dev\tribler\src\tribler-gui\tribler_gui\core_manager.py", line 67, in on_core_finished
    self.on_finished()
  File "C:\dev\tribler\src\tribler-gui\tribler_gui\core_manager.py", line 132, in on_finished
    self.tribler_stopped.emit()
RuntimeError: wrapped C/C++ object of type CoreManager has been deleted

Now, this error is gone.

I don't know, will the PR fix crashes on Mac described in #6564 or not, but I hope it should also fix them as well.

The reason for the problem "wrapped C/C++ object of type CoreManager has been deleted" was emitting the CoreManager.tribler_stopped signal at the moment when the application had already quit, and Qt internal structures were partially destroyed. As it turns out, nobody actually uses this signal, so I removed it. If necessary, we can reintroduce it in the future, as now the improved logic of the CoreManager's shutdown process should track do we already call QApplication.quit() or not and set the corresponding flag quitting_app to True, preventing some actions at this stage.

I rewrote the logic of the CoreManager shutdown to make it easier to understand. During this process, I noticed that we potentially do not always process errors on the Core shutdown. This is the previous logic of error handling:

def on_core_finished(self, exit_code, exit_status):
        if self.shutting_down and self.should_stop_on_shutdown:
            self.on_finished()
        elif not self.shutting_down and exit_code != 0:
            ...
            raise CoreCrashedError(exception_message)

Note that if CoreManager was not in the shutting_down state and the Core process suddenly finished with exit code 0, this situation was not processed. I don't know how often it can happen in practice, but in my opinion, if Core suddenly finished with exit code 0 when GUI did not expect it, it should be treated as an error, reported Sentry, and then GUI should stop. Before this PR, GUI hangs indefinitely in this situation.

Now the logic looks the following way:

def on_core_finished(self, exit_code, exit_status):
        self.core_running = False
        self.core_finished = True
        if self.shutting_down:
            if self.should_quit_app_on_core_finished:
                self.quit_application()
        else:
            ...
            raise CoreCrashedError(error_message)

That is, now GUI handles any unexpected Core process termination.

I renamed CoreManager state flags because the previous names do not reflect the actual flag meaning. Also, I added some new flags to describe the CoreManager state completely.

@kozlovsky kozlovsky requested review from devos50 and drew2a and removed request for devos50 and drew2a December 3, 2021 12:42
@devos50
Copy link
Contributor

devos50 commented Dec 4, 2021

retest this please

@devos50
Copy link
Contributor

devos50 commented Dec 4, 2021

The GUI tests still seem to fail on Windows, see here. The tag dialog test also fails on Linux - I will probably refactor that one soon.

@kozlovsky kozlovsky force-pushed the fix/gui_tests_stability branch from 29df8ee to 29af36e Compare December 6, 2021 10:56
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 6, 2021

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.0% 0.0% Duplication

@kozlovsky
Copy link
Contributor Author

retest this please

@kozlovsky kozlovsky marked this pull request as ready for review December 6, 2021 11:59
@kozlovsky kozlovsky requested review from a team, drew2a and devos50 and removed request for a team December 6, 2021 11:59
@devos50
Copy link
Contributor

devos50 commented Dec 6, 2021

@kozlovsky is this related to your PR? (build documentation job)

Traceback (most recent call last):
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinx/cmd/build.py", line 276, in build_main
    app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinx/application.py", line 237, in __init__
    self.setup_extension(extension)
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinx/application.py", line 394, in setup_extension
    self.registry.load_extension(self, extname)
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinx/registry.py", line 429, in load_extension
    mod = import_module(extname)
  File "/usr/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 848, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinxcontrib/openapi/__init__.py", line 13, in <module>
    from sphinxcontrib.openapi import renderers, directive
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinxcontrib/openapi/renderers/__init__.py", line 4, in <module>
    from ._httpdomain_old import HttpdomainOldRenderer
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinxcontrib/openapi/renderers/_httpdomain_old.py", line 6, in <module>
    from .. import openapi20, openapi30, utils
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinxcontrib/openapi/openapi20.py", line 16, in <module>
    from sphinxcontrib.openapi import utils
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/sphinxcontrib/openapi/utils.py", line 20, in <module>
    from m2r import convert as convert_markdown
  File "/home/jenkins/workspace/GH_Tribler_PR_Tests/PR_build_docs/venv/lib/python3.8/site-packages/m2r.py", line 59, in <module>
    class RestBlockGrammar(mistune.BlockGrammar):
AttributeError: module 'mistune' has no attribute 'BlockGrammar'

@kozlovsky
Copy link
Contributor Author

@devos50 it should not be related, but I'll investigate that.

The previous error

    raw_output = bytes(self.core_process.readAllStandardOutput())
RuntimeError: wrapped C/C++ object of type QProcess has been deleted

should be fixed now.

@kozlovsky kozlovsky merged commit 2741b94 into Tribler:main Dec 6, 2021
@kozlovsky kozlovsky deleted the fix/gui_tests_stability branch December 6, 2021 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants