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

PR: Add workflow to run tests with PyQt6 (CI) #23118

Merged
merged 9 commits into from
Feb 18, 2025

Conversation

rear1019
Copy link
Contributor

@rear1019 rear1019 commented Nov 29, 2024

Description of Changes

This is a draft to add PyQt6/PySide6 to the CI as suggested in PR #22620:

  • The workflow file based on the existing one for Linux.
  • For now the tests run on Linux only.
  • Only pip-based installation is used as conda has no PyQt6 package yet.
  • The tests don’t run at all with PySide6 (SEGFAULT at collection phase of pytest). I can reproduce it locally, but haven’t looked into it.
  • The mostly run with PyQt6, however, there is a SEGFAULT at the end (haven’t looked into it).

Issue(s) Resolved

Part of #20201.

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

rear1019

@rear1019
Copy link
Contributor Author

rear1019 commented Jan 7, 2025

@ccordoba12 I have added two PyQt6 fixes to this PR. There are still failures I haven’t looked into; see [1] for the most current CI run as of today. Nevertheless, I think we could consider merging the PR in order to move forward with Qt 6 support. Note there are some review comments [2].

Regarding PySide6 support: In [3] you mentioned that you have PySide6 related fixes. Would you mind providing them? We are more than happy to help the project with the Qt 6 transition (both PySide6 and PyQt6).

PS: You may have noticed that we provided less patches in 2024 than in the years before. This is because we had a stable experience with v5.4, and especially so with v5.5. Thank you and the whole dev team very much for this great project ❤️

[1] https://github.com/procitec/spyder/actions/runs/12652008190
[2] #23118 (review)
[3] #22620

@ccordoba12
Copy link
Member

ccordoba12 commented Jan 8, 2025

There are still failures I haven’t looked into; see [1] for the most current CI run as of today.

I'm really happy to see that there are really few failures! That's awesome!

Nevertheless, I think we could consider merging the PR in order to move forward with Qt 6 support. Note there are some review comments [2].

Agreed. Please skip the failing tests for Qt6 and post a list of them in a comment here. Then I'll merge.

Regarding PySide6 support: In [3] you mentioned that you have PySide6 related fixes. Would you mind providing them?

I'll try to do it soon (hopefully next week). But I think it'd be better to comment the PySide6 CI slots for now and work on support for it after my PR is merged.

What do you think?

We are more than happy to help the project with the Qt 6 transition (both PySide6 and PyQt6).

Thanks a lot! That will help us to provide more features in 6.1 for you and all other users.

PS: You may have noticed that we provided less patches in 2024 than in the years before. This is because we had a stable experience with v5.4, and especially so with v5.5. Thank you and the whole dev team very much for this great project ❤️

Thanks for your kind words. We're really happy to hear that your experience with Spyder has improved significantly in the last two years.

@ccordoba12
Copy link
Member

About the failure in test_dependencies_for_spyder_setup_extras_requires_in_sync, please skip checking pywin32 on it. That's the best we can do for now because conda environment yaml files don't handle selectors (i.e. packages for specific platforms).

@ccordoba12
Copy link
Member

@rear1019, what's missing for this one to be merged now that CIs are green? Only reverting the things you mentioned above?

@rear1019
Copy link
Contributor Author

@ccordoba12 Sorry for the slow response. I wasn’t able to work on this PR in the last few weeks due to personal circumstances.

what's missing for this one to be merged now that CIs are green? Only reverting the things you mentioned above?

The CI executed in this PR doesn’t include the Qt 6 workflow file from this PR. I think this is because the workflow file exists in our fork only. See here [1] for CI runs with Qt 6.

There are still some failures. I wasn’t able to reproduce them locally last week. I will look into them gain this week.

[1] https://github.com/procitec/spyder/actions/workflows/test-linux-qt6.yml

@ccordoba12
Copy link
Member

Thanks for the update @rear1019! Looking forward to your progress on fixing the tests that are failing.

@rear1019
Copy link
Contributor Author

There are still some failures. I wasn’t able to reproduce them locally last week. I will look into them gain this week.

I can now reproduce the issue locally: There is a SEGFAULT when runtest.py exits. Looking at the backtrace it might be an issue in PyQt6. Or Spyder might be handling QThreads wrongly. I will try to investigate further this week.

@ccordoba12 In the meantime, would you mind providing your patches for PySide6? We are more than happy to help finish the work.

@rear1019 rear1019 force-pushed the qt6_ci branch 3 times, most recently from 72907b2 to 35646e5 Compare February 14, 2025 15:25
@ccordoba12
Copy link
Member

In the meantime, would you mind providing your patches for PySide6? We are more than happy to help finish the work.

Done in PR #23732.

@rear1019
Copy link
Contributor Author

In the meantime, would you mind providing your patches for PySide6? We are more than happy to help finish the work.

Done in PR #23732.

Thank you very much! I will look into the patches next week.

Regarding the SEGFAULT mentioned before: My investigation yielded some positive results. I will finalize the PR next week.

@rear1019 rear1019 force-pushed the qt6_ci branch 2 times, most recently from f9b90d8 to 43bec31 Compare February 17, 2025 12:26
Notes:

- For now the tests run on Linux only. The workflow file based on the
  existing one for Linux.

- Only pip-based installation is used as conda has no PyQt6 package yet.

- Only PyQt6 is used as PySide6 is not supported yet.
Fix 'extras_require' in setup.py to fix the warning. This requires fixes
for the parsing of dependencies in test_dependencies_in_sync.py: Use the
'packaging' module which simplifies the code.
Set parent object for QTimer and don’t use singleShot() static method.
This prevents the timer from being fired after the parent object is
destroyed.

The issue has been discovered with PyQt6 by test [1].

[1] spyder/plugins/editor/extensions/tests/test_closequotes.py::test_close_quotes
Some tests explicitly close the editorstack widget. qtbot also closes
the widget after the test finishes. The latter action may fail when
"delete on close" flag is set => reset the flag.
- Add exception handling and exit thread loop when EOF is received

- Start thread _after_ connecting signals
Disconnect all signal-slots connections of the main QThread just before
runtests.py exits. This prevents a SEGFAULT when the finished signal of
the main QThread is triggered. This is an issue in the PyQt6 bindings
(PySide6 is also affected).
@rear1019 rear1019 marked this pull request as ready for review February 17, 2025 13:33
@rear1019
Copy link
Contributor Author

@ccordoba12, I think this is ready to be merged. Comments from this [1] review are resolved. Some tests are disabled (see commit “tests: Skips tests failing with PyQt6 when PyQt6 is used”). Nevertheless, I think we should consider merging the PR in order to move forward with Qt 6 support.

[1] #23118 (review)

@ccordoba12 ccordoba12 changed the title PR: CI: Add workflow to run tests with PyQt6/PySide6 PR: CI: Add workflow to run tests with PyQt6 Feb 17, 2025
@ccordoba12 ccordoba12 changed the title PR: CI: Add workflow to run tests with PyQt6 PR: Add workflow to run tests with PyQt6 (CI) Feb 17, 2025
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

A small comment for you, the rest looks good to me @rear1019.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work on this @rear1019!

@ccordoba12 ccordoba12 merged commit d9b31d6 into spyder-ide:master Feb 18, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants