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

Add unit test to Clock.get_rawtime #1939

Merged
merged 1 commit into from Jun 11, 2020
Merged

Add unit test to Clock.get_rawtime #1939

merged 1 commit into from Jun 11, 2020

Conversation

jiquach
Copy link
Contributor

@jiquach jiquach commented Jun 10, 2020

Closes #1786

Tests the logic:

  • Clock initiation.
  • get_rawtime()gets actual elapse time with a framerate argument in tick.
  • get_rawtime() and get_time() return the same thing when there is no framerate argument.

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.

Looks good to me!

As with all the time module tests the delta value may end up needing to be tweaked for CI, but it's hard to tell without running it on CI lots of times.

Thanks for the contribution!

Appveyor is accidentally down at the minute so I won't merge this yet.

@illume illume closed this Jun 10, 2020
@illume illume reopened this Jun 10, 2020
@illume
Copy link
Member

illume commented Jun 10, 2020

This failed on one of the appveyor jobs...

======================================================================
FAIL: test_get_rawtime (pygame.tests.time_test.ClockTypeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python27-x64\lib\site-packages\pygame\tests\time_test.py", line 55, in test_get_rawtime
    self.assertAlmostEqual(delay_miliseconds, c1, delta=delta)
AssertionError: 100.0 != 117 within 15 delta

@jiquach
Copy link
Contributor Author

jiquach commented Jun 10, 2020

@MyreMylar @illume Thanks for the reviews. I saw the AppVeyor test and saw that the common delta is 50 ms in the module. I will change that delta and rebase the branch.

@jiquach
Copy link
Contributor Author

jiquach commented Jun 11, 2020

It seems that one of the AppVeyor jobs failed on a different test (test_get_fps). I checked out a couple of other AppVeyor builds and it seems that the specific test passes for those.

  • Is there a way to kick off another AppVeyor build to confirm whether it is a valid issue to look into?
  • The test_get_fps uses a 20% delta from the true FPS vs the calculated FPS from get_fps() which is large already so increasing the delta might make the test less concrete.

@illume
Copy link
Member

illume commented Jun 11, 2020

Hi @jiquach thanks for mentioning the get_fps fail. I made a note about it at the issue here: #1785

The problem with the VMs and containers on highly loaded systems is that they can get very small time slices sometimes, and take quite a while to come back from sleeps. Making it pretty difficult to test timing related things.

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.

👍 🎉 thank you!

@illume illume merged commit e758a43 into pygame:master Jun 11, 2020
@illume illume added the time pygame.time label Sep 17, 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_rawtime()
3 participants