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

Unit tests for time.Clock.get_time() #1909

Merged
merged 1 commit into from Jun 5, 2020
Merged

Unit tests for time.Clock.get_time() #1909

merged 1 commit into from Jun 5, 2020

Conversation

pedrodelapena
Copy link
Contributor

@pedrodelapena pedrodelapena commented Jun 3, 2020

Closes #1787

@pedrodelapena pedrodelapena changed the title Unit tests for time.Clock.get_time() [WIP] Unit tests for time.Clock.get_time() Jun 3, 2020
@pedrodelapena pedrodelapena changed the title [WIP] Unit tests for time.Clock.get_time() Unit tests for time.Clock.get_time() Jun 4, 2020
Copy link
Contributor

@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.

This test currently takes 3 minutes to run on it's own, that's longer than all the other tests combined.

The problem with that is that since the CI on Appveyor runs each build in sequence... well it basically triples the amount of time the CI takes to run and (as I discovered before when I enabled /analyze on every build). Appveyor can quickly get backed up on busy days with multiple pull requests waiting to have their go.

I think the time delay needs to be reduced a lot to something like:

time.sleep(.100)

then the test should be more like 3 seconds long and should still achieve the same goal.

Thanks for the contribution!

@MyreMylar
Copy link
Contributor

I'm slightly nervous about reducing the error delta as we just had two time module tests fail on CI today because their timing error delta was too small - but perhaps it'll be fine.

@pedrodelapena
Copy link
Contributor Author

I'm slightly nervous about reducing the error delta as we just had two time module tests fail on CI today because their timing error delta was too small - but perhaps it'll be fine.

I'll revert the changes on the delta parameter and I'll squash the commits. The error delta I originally used was too small while the delay I used was too big, which lead me to increase the first later on. I hope this change gets it all right!

@MyreMylar
Copy link
Contributor

LGTM 👍

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Thanks :)

@illume illume merged commit baaa34f into pygame:master Jun 5, 2020
@pedrodelapena pedrodelapena deleted the UT_clock.get_time() branch June 5, 2020 17:24
@illume illume added the time pygame.time label Sep 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
time pygame.time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit test: time.Clock.get_time()
3 participants