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

[CI] Enable audio tests in Firefox #23701

Merged
merged 10 commits into from
Feb 24, 2025

Conversation

cwoffenden
Copy link
Contributor

@cwoffenden cwoffenden commented Feb 19, 2025

Following on from #23665, a dummy audio device is now enabled for Firefox in the CI.

Multiple attempts were tried (adding Alsa's snd-dummy and snd-aloop), eventually getting PulseAudio to work.

The output from the Firefox runs was manually verified that the tests had run (if they hadn't, due to work being run in the AudioWorklet callback, they'd have timed out and failed).

(Comment edited with the choice of PulseAudio now this works)

@cwoffenden cwoffenden marked this pull request as draft February 19, 2025 11:44
@sbc100
Copy link
Collaborator

sbc100 commented Feb 19, 2025

The circleCI runners support "retry with SSH" and you can log into them directly (in case that helps)

@cwoffenden
Copy link
Contributor Author

The circleCI runners support "retry with SSH" and you can log into them directly

I'll need to create an account and look at it, but it's taking me away from what I'm actually trying to do.

I may still quickly try adding the PulseAudio package (which can run in the user account, AFAIK), but I'm going to need to come back to it.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 19, 2025

Feel free to de-prioritize this if you other things you are working on. We've be operating without any audio testing in CI for over 10 years now so a little more delay shouldn't be a problem.

@cwoffenden
Copy link
Contributor Author

Tomorrow I'll make #23659 work with the now working CI, specifically making test_audio_worklet_params_mixing run as part of the browser tests, since it covers most of the AW API (the other tests cover features and are really interactive, this single tests proves out the audio API pretty much as a whole).

Then I'm sitting on the code that fixes wasm64 and 2GB for audio.

@cwoffenden
Copy link
Contributor Author

Testing again locally, adding PulseAudio with its dummy mixer works, then starting it for the user:

apt-get install -q -y pulseaudio
pulseaudio --start

The CI machine seems to start installing this but fails with gstreamer and its dependencies:

https://app.circleci.com/pipelines/github/emscripten-core/emscripten/40699/workflows/ce65623e-e7d3-469f-a7e9-7c766d3a554b/jobs/903792?invite=true#step-108-1722_106

@cwoffenden cwoffenden force-pushed the cw-audio-run-firefox-tests branch from 49fe005 to f3190ee Compare February 23, 2025 08:15
@cwoffenden
Copy link
Contributor Author

Woo-hoo! They're running:

https://app.circleci.com/pipelines/github/emscripten-core/emscripten/40706/workflows/d392b6a1-ea7b-41df-b410-ed763a9909d9/jobs/903939?invite=true#step-109-12288_96

I'd like to move the install to either its own section or with the Firefox installer, and possibly look at the warnings given (PulseAudio's not meant to be run as root).

@cwoffenden cwoffenden changed the title [CI] Try enabling audio tests in Firefox [CI] Enable audio tests in Firefox Feb 23, 2025
@cwoffenden cwoffenden marked this pull request as ready for review February 23, 2025 17:45
@cwoffenden cwoffenden force-pushed the cw-audio-run-firefox-tests branch from d9fa21e to 81c711e Compare February 24, 2025 17:43
# This should add and start PulseAudio's dummy mixer. It will warn
# that "This program is not intended to be run as root" but it can
# be ignored.
apt-get update -y; true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why ; true there? Does apt-get update fail for some reason?

Copy link
Contributor Author

@cwoffenden cwoffenden Feb 24, 2025

Choose a reason for hiding this comment

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

Yes, it's just in case it fails. The install is the important one, which if that fails then the test will definitely fail, so I was trying to reduce any flakiness.

(And the install failing depends on how old the image is. Install without an update currently fails on CircleCI due to the package URLs being out of date, but doesn't on my Debian VM.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that should be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % comment

@sbc100 sbc100 enabled auto-merge (squash) February 24, 2025 18:36
@sbc100 sbc100 merged commit 15967b4 into emscripten-core:main Feb 24, 2025
29 checks passed
@cwoffenden cwoffenden deleted the cw-audio-run-firefox-tests branch February 24, 2025 19:57
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.

2 participants