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

[test] AudioWorklet tests now correctly run and exit #23695

Merged
merged 17 commits into from
Feb 20, 2025

Conversation

cwoffenden
Copy link
Contributor

@cwoffenden cwoffenden commented Feb 18, 2025

All the audio tests in test_browser.py (except the mixer) were updated now the CI has audio, specifically:

  • Tests that require audio were flagged as so (tests that don't are documented)
  • Tests that currently fail for 2GB and wasm64 were flagged (only tests that play audio fail)
  • All tests were migrated to btest_exit() as previously discussed
  • (Tests moved printf calls to Emscripten's API and not stdio)
  • All tests were run manually and the audio output verified
  • All tests were run with emscripten_force_exit returning non-zero to verify they failed
  • All of this was also verified that they're running on a Debian VM without audio to manually check the console for "Test success" and other outputs for tests without requires_sound_hardware

The mixer/AW struct test is a standalone PR in #23659.

Fixes: #23131

cwoffenden added a commit to cwoffenden/emscripten that referenced this pull request Feb 18, 2025
sbc100 pushed a commit that referenced this pull request Feb 18, 2025
Step one of a fix for #23131 (the second part in #23695).

Enabled Chrome's `FakeAudioOutputStream` for the CI machines and
bypassed the need for user interaction. The Chrome tests are now enabled
with `@requires_sound_hardware`.

Many attempts at replicating the same functionally were tried with
Firefox (details below) but audio would never run so its
`EMTEST_LACKS_SOUND_HARDWARE` was left unchanged.
@cwoffenden cwoffenden force-pushed the cw-audio-test-must-run branch 2 times, most recently from 0b97ea5 to fd8b9ba Compare February 18, 2025 22:21
@cwoffenden cwoffenden marked this pull request as draft February 18, 2025 22:33
@cwoffenden
Copy link
Contributor Author

@sbc100 Would you prefer all of the AW tests working/enabled in batches like this, or all as one?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 19, 2025

@sbc100 Would you prefer all of the AW tests working/enabled in batches like this, or all as one?

How many AW tests are there? If all of the tests are to be changes like this in the same manor I think doing it all at once seems reasonable.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 19, 2025

If you are asking about landing the fixes at the same time as the test updates, I think those are good to keep separate. I

I like this this change simply updates test code, for example.

@sbc100 sbc100 changed the title [CI] AudioWorklet test runs and exits [test] AudioWorklet test runs and exits Feb 19, 2025
@cwoffenden
Copy link
Contributor Author

Mostly just the small collection of AW tests in the same section of code, and they should follow the same pattern. I’ll combine them together.

I’ll keep the test with the audio files and mixer part of the separate current PR (because it’s larger).

And again separately I’ll add this to the browser tests but disabled, since it’s broken:

https://github.com/emscripten-core/emscripten/blob/main/test/webaudio/audioworklet_emscripten_futex_wake.cpp

And any fixes, e.g. wasm64 and 2gb in their own PRs.

@cwoffenden cwoffenden force-pushed the cw-audio-test-must-run branch from 1cd9f52 to 555ccd4 Compare February 20, 2025 15:20
@cwoffenden cwoffenden changed the title [test] AudioWorklet test runs and exits [test] AudioWorklet tests runs and exits Feb 20, 2025
@cwoffenden cwoffenden marked this pull request as ready for review February 20, 2025 16:36
@cwoffenden cwoffenden changed the title [test] AudioWorklet tests runs and exits [test] AudioWorklet tests now correctly run and exit Feb 20, 2025
cwoffenden added a commit to cwoffenden/emscripten that referenced this pull request Feb 20, 2025
cwoffenden added a commit to cwoffenden/emscripten that referenced this pull request Feb 20, 2025
@sbc100
Copy link
Collaborator

sbc100 commented Feb 20, 2025

Is this change ready to land now?

@cwoffenden
Copy link
Contributor Author

Is this change ready to land now?

Yes, this change is all done (as is the mixer PR).

@sbc100 sbc100 merged commit 184834f into emscripten-core:main Feb 20, 2025
28 of 29 checks passed
@cwoffenden cwoffenden deleted the cw-audio-test-must-run branch February 20, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AUDIO_WORKLET] tests in the CI don't appear to be run (in test_browser)
2 participants