Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 12, 2019

Also add basic sdl2_mixer test outside of browser tests that just
tests for successful compilation.

Fixes #10015

@sbc100 sbc100 requested a review from kripken December 12, 2019 20:17
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I think I mentioned in another PR or issue, I think a better direction would be to compile but not run the audio tests. That is, we currently say our CI lacks audio, so we skip those tests, but instead we could still build them, just not run them.

There is precedent for that, if too many browser tests are unresponsive then we keep building, but stop running (as we assume the browser is "stuck"). That happens at the top of run_browser. Perhaps we could add a similar mechanism for audio and other things?

for f in glob.glob(os.path.join(src_dir, pattern)):
logger.debug(os.path.join(dest, os.path.basename(f)))
matches = glob.glob(os.path.join(src_dir, pattern))
assert matches
Copy link
Member

Choose a reason for hiding this comment

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

perhaps add an error message? otherwise the error will be unclear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an assertion, for developers only. No user should ever see this, its for debugging while building/modifying ports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But i'll add a message anyway :)

Copy link
Collaborator Author

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

Personally I prefer a test that doesn't live in test_browser at all. Then I can run it locally without launching a browser at all.

I'm happy to have the audio tests build-only during browser tests, but I think we should have at least one test in test_other for each of out ports. I consider those two things orthogonal.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Fair enough, agreed on both counts.

Also add basic sdl2_mixer test outside of browser tests that just
tests for successful compilation.

Fixes #10015
@sbc100 sbc100 merged commit deed1c5 into incoming Dec 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix_sdl_mixer branch December 16, 2019 21:58
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
…ten-core#10021)

Also add basic sdl2_mixer test outside of browser tests that just
tests for successful compilation.

Fixes emscripten-core#10015
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.

'SDL2/SDL_mixer.h' file not found

3 participants