-
-
Notifications
You must be signed in to change notification settings - Fork 154
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
Fix midi timer reset when quit #3018
Conversation
test/midi_test.py
Outdated
self.assertTrue(0 <= mtime < 100) | ||
if "pytest" in sys.modules: | ||
# under pytest the midi module takes a couple of seconds to init | ||
# instead of milliseconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why this is happening. How does using pytest slow down midi?
Absolutely no idea!
…On Sun, 28 Jul 2024, 12:12 Ankith, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/midi_test.py
<#3018 (comment)>
:
> @@ -319,8 +320,14 @@ def test_get_init(self):
def test_time(self):
mtime = pygame.midi.time()
self.assertIsInstance(mtime, int)
- # should be close to 2-3... since the timer is just init'd.
- self.assertTrue(0 <= mtime < 100)
+ if "pytest" in sys.modules:
+ # under pytest the midi module takes a couple of seconds to init
+ # instead of milliseconds
I'm curious why this is happening. How does using pytest slow down midi?
—
Reply to this email directly, view it on GitHub
<#3018 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADGDGGXHAJE5YOGTRFYKTXDZOUQ7ZAVCNFSM6AAAAABLOPX4GGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBTGU2DANRZGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this even though this has left me confused. Either way, the non-pytest test case is staying the same so I don't think this should cause a problem
This issue is because import pygame
import pygame.midi
pygame.midi.init()
pygame.time.wait(200)
print(pygame.midi.time())
pygame.midi.quit()
pygame.midi.init()
print(pygame.midi.time()) I think the best option here would be to wait x milliseconds and test if |
& undo test changes
Based on this, I tired calling PortMidi's timer stop function, This seem to resolve the issue with pytest for me and seems to make the timing behave as per the docs, and logically (stopping the submodule timer when we quit the submodule). So I'm going to propose this as the alternative fix. The docs for
|
Test program: from time import sleep
import pygame.midi
pygame.midi.init()
sleep(0.05)
print("time since midi.init():", pygame.midi.time())
pygame.midi.quit()
pygame.midi.init()
print("time since midi.init():", pygame.midi.time())
sleep(0.05)
print("time since midi.init():", pygame.midi.time()) Output, before this PR:
Output, after this PR:
|
pytest test -s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I tested this fix locally and it works. I have renamed the title of the PR to reflect the latest changes.
Thanks for the PR 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally as well, works as it should. Thanks for the PR!
A little while ago @ankith26 changed something to make pytest work better with out test suite. I like the output of pytest so I was trying it out today and, after a little fidddling I realised we need to run it with the
-s
flag, like so:to make all the command line unit tests work.
Anyway, with that they all seem to work OK through pytest, bar two tests. This 'how much time has passed since
midi.init()
was called was one of them - for some reason it takes at least 10 times as long to go frompygame.midi.init()
topygame.midi.time()
when running the test through pytest. I'm not sure if we want to cater to pytest by making a small change to this test or not, but anyway I though I'd submit a PR.The test for
set_gamma_ramp()
also doesn't pass now - I think it should probably be removed as the function is deprecated in pygame, the corresponding SDL function is also deprecated and it doesn't work at all on my PC at all anymore - I expect windows has removed support for it.The other thing running pytest does is show all the deprecated functions we are testing in the tests without supressing the generated warnings - this make the test output a bit messy and harder to spot deprecation warnings not coming from our test code.
Warnings output from pytest test -s