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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add unitest for Window #2230

Merged
merged 15 commits into from
Aug 28, 2023
Merged

Add unitest for Window #2230

merged 15 commits into from
Aug 28, 2023

Conversation

yunline
Copy link
Contributor

@yunline yunline commented Jun 5, 2023

No description provided.

@yunline yunline added tests tests (module) video pygame.video labels Jun 5, 2023
Starbuck5
Starbuck5 reviewed Jun 18, 2023
View reviewed changes
test/window_test.py
Comment on lines +14 to +17
@unittest.skipIf(
os.environ.get("SDL_VIDEODRIVER") == "dummy",
"requires the SDL_VIDEODRIVER to be a non dummy value",
)
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to get as many of these running without this check if possible.

This check means the test won't run on CI.

Copy link
Member

@Starbuck5 Starbuck5 Aug 27, 2023 •
edited

Choose a reason for hiding this comment

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

I tested some of these skipped methods without SDL_VIDEODRIVER=dummy set and they didn't crash or anything, so we should not skip them.

@Matiiss Matiiss mentioned this pull request Jul 30, 2023
Merged
@yunline yunline marked this pull request as ready for review August 5, 2023 06:13
@yunline yunline requested a review from a team as a code owner August 5, 2023 06:13
MyreMylar
MyreMylar approved these changes Aug 14, 2023
View reviewed changes
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM. Tests all run locally and seem reasonable 馃憤

Looks like about 60% of them can now run on the CI. kind of inevitable that some of them won't be runnable with the display-less CI machines. Another reminder of why it is good to regularly test PRs locally.

@MyreMylar MyreMylar added this to the 2.3.2 milestone Aug 14, 2023
dr0id
dr0id approved these changes Aug 15, 2023
View reviewed changes
Copy link
Contributor

@dr0id dr0id left a comment

Choose a reason for hiding this comment

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

run tests too locally

@yunline yunline modified the milestones: 2.3.2, 2.4.0 Aug 22, 2023
yunline and others added 5 commits August 22, 2023 18:40
@yunline
Merge branch 'main' into window-unitest
606cfe3
@yunline
Update window tests
29847dc 
Add tests for maximum_size and minimum_size
@yunline
Add version compat for tests
79d7f64
@Starbuck5
Add test variants that can run on CI
b783dc8 
This is so the existence of the attributes and their return types is still tested on CI, even if the full set/get path doesn't work with SDL_VIDEODRIVER=dummy
@Starbuck5
Fix illegal operation
a94b873
Starbuck5
Starbuck5 approved these changes Aug 28, 2023
View reviewed changes
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Alright, I added some additional tests to get around the CI videodriver, for some added peace of mind when running tests on CI.

Thanks for working on this yunline! If my edits look good to you, you're free to merge.

@yunline yunline merged commit 578ed7d into pygame-community:main Aug 28, 2023
31 checks passed
@yunline yunline deleted the window-unitest branch August 28, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankith26 ankith26 ankith26 left review comments

@dr0id dr0id dr0id approved these changes

@MyreMylar MyreMylar MyreMylar approved these changes

@Starbuck5 Starbuck5 Starbuck5 approved these changes

Assignees
No one assigned
Labels
tests tests (module) video pygame.video
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yunline @dr0id @MyreMylar @Starbuck5 @ankith26