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.

Sign up for GitHub

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

Jump to bottom

PR: Add missing tests for aliased methods #317

Merged
merged 13 commits into from
Jan 31, 2022
Merged

PR: Add missing tests for aliased methods #317

merged 13 commits into from
Jan 31, 2022

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Jan 20, 2022 •
edited
Loading

Fixes #316

New tests for:

  • QtCore
  • QtGui
  • QtPrintSupport
  • QtSql
  • QtWidgets

@dalthviz dalthviz added this to the v2.0.1 milestone Jan 20, 2022
@dalthviz dalthviz self-assigned this Jan 20, 2022
@dalthviz dalthviz force-pushed the fixes_issue_316 branch 3 times, most recently from 96b71f5 to 5056678 Compare January 24, 2022 18:56
@CAM-Gerlach
Copy link
Member

Seems like just a few random Qt segfaults on a few specific tests on a couple platforms...

@dalthviz dalthviz changed the title [WIP] PR: Add missing tests for aliased methods PR: Add missing tests for aliased methods Jan 25, 2022
@dalthviz dalthviz force-pushed the fixes_issue_316 branch 2 times, most recently from f05fb0e to 327f52d Compare January 25, 2022 18:32
@dalthviz dalthviz marked this pull request as ready for review January 26, 2022 14:53
@kumattau
Copy link
Contributor

hi, @dalthviz

Thank you for testing.

I think subclass tests are needed to check that the original problem is fixed.

#305 (comment)

This leads to an error if I call exec_ in a QDialog subclass, at least with PyQt6
exec(self): first argument of unbound method must have type 'QDialog'

@dalthviz dalthviz force-pushed the fixes_issue_316 branch 2 times, most recently from 351955b to 0ef6f10 Compare January 26, 2022 16:26
CAM-Gerlach
CAM-Gerlach requested changes Jan 27, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks @dalthviz ! I had a few comments/suggestions, mostly substantive ones as opposed to nitpicks.

.github/workflows/test.sh Outdated Show resolved Hide resolved
qtpy/tests/test_qtsql.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtsql.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
qtpy/tests/test_qtsql.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
@dalthviz @CAM-Gerlach
Testing: Apply suggestions from code review
49c0dbd 
* Change skip constrains definition (`os.name` vs `sys.platform`)
* `pytest` version constraints
* Syntaxis to select `pytest-qt` version to install on the CIs

Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@dalthviz dalthviz force-pushed the fixes_issue_316 branch 3 times, most recently from 8e3f052 to 4b9c2ca Compare January 27, 2022 18:11
@dalthviz dalthviz requested a review from CAM-Gerlach January 27, 2022 18:53
CAM-Gerlach
CAM-Gerlach approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM! Thanks @dalthviz !

qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
ccordoba12
ccordoba12 reviewed Jan 28, 2022
View reviewed changes
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.

This looks pretty good to me, thanks @dalthviz!

Apart from the small formatting nitpicks I left for you, I also suggest to do a small refactoring to simplify the skip conditions you added to several tests here.

qtpy/tests/test_qtcore.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Show resolved Hide resolved
qtpy/tests/test_qtprintsupport.py Show resolved Hide resolved
qtpy/tests/test_qtsql.py Show resolved Hide resolved
qtpy/tests/test_qtsql.py Show resolved Hide resolved
qtpy/tests/test_qtsql.py Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Show resolved Hide resolved
dalthviz and others added 2 commits January 28, 2022 10:18
@dalthviz @ccordoba12 @CAM-Gerlach
Testing: Apply suggestions from code review
2e4d7dd 
* Improve test files docstrings and add blank lines at the end of files
* Use `Path.exists` instead of `os.path.exists`

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@dalthviz
Testing: Create utils module for USE_CONDA validations
348710c
ccordoba12
ccordoba12 reviewed Jan 28, 2022
View reviewed changes
qtpy/tests/utils.py Show resolved Hide resolved
qtpy/tests/utils.py Show resolved Hide resolved
ccordoba12
ccordoba12 reviewed Jan 29, 2022
View reviewed changes
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.

Last style suggestions for you @dalthviz, then this should be ready.

qtpy/tests/test_qtcore.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtprintsupport.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
CAM-Gerlach
CAM-Gerlach reviewed Jan 29, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Fix a few syntax errors in @ccordoba12 's suggestions; otherwise looks good. Thanks @dalthviz !

qtpy/tests/test_qtprintsupport.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtwidgets.py Outdated Show resolved Hide resolved
@dalthviz @ccordoba12 @CAM-Gerlach
Testing: Apply suggestions from code review
328f82a 
* Improve skip statements code style

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@dalthviz dalthviz force-pushed the fixes_issue_316 branch 2 times, most recently from 0d8ff1a to 1edfc07 Compare January 31, 2022 16:06
@dalthviz
Testing: Add restriction for pip version with Python 3.6 to prevent e…
4a29566 
…rror with the dataclasses module when building wheel
ccordoba12
ccordoba12 approved these changes Jan 31, 2022
View reviewed changes
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.

Great work, thanks @dalthviz!

CAM-Gerlach
CAM-Gerlach approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dalthviz !

@CAM-Gerlach CAM-Gerlach merged commit 6f5a08e into spyder-ide:master Jan 31, 2022
@CAM-Gerlach CAM-Gerlach mentioned this pull request Feb 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

@ccordoba12 ccordoba12 ccordoba12 approved these changes

Assignees

@dalthviz dalthviz

Labels
type:Task
Projects
None yet
Milestone
v2.0.1
Development

Successfully merging this pull request may close these issues.

Tests for instance methods alias mapping fix (exec_ vs exec and others)
4 participants
@dalthviz @CAM-Gerlach @kumattau @ccordoba12