Skip to content

[test] Remove zzz prefix from high memory browser tests #20761

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

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 21, 2023

  • remove unneeded require_wasm64 hack (the default node version we
    ship now supports wasm64 so it is no longer needed)
  • remove unneeded requires_v8 from high memory tests
  • remove unneeded @no_wasm64 from test_4gb_fail

@sbc100 sbc100 requested a review from kripken November 21, 2023 17:16
@sbc100 sbc100 force-pushed the zzz_browser_tests branch 2 times, most recently from dd87c5a to 78a05f1 Compare November 21, 2023 17:34
- remove unneeded `require_wasm64` hack (the default node version we
  ship now supports wasm64 so it is no longer needed)
- remove unneeded `requires_v8` from high memory tests
- remove unneeded `@no_wasm64` from test_4gb_fail
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.

The zzzs were to defer allocating huge amounts of memory to the end, so any OOM there wouldn't affect anything else. I guess these tests have been passing stably for a while now, so it's ok to change that ordering?

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 21, 2023

The zzzs were to defer allocating huge amounts of memory to the end, so any OOM there wouldn't affect anything else. I guess these tests have been passing stably for a while now, so it's ok to change that ordering?

I think so yes.. are you saying that once a large allocation fails a small allocation might also fail? that doesn't make a lot of sense to me.

@sbc100 sbc100 merged commit bbb2e36 into emscripten-core:main Nov 21, 2023
@sbc100 sbc100 deleted the zzz_browser_tests branch November 21, 2023 22:17
@kripken
Copy link
Member

kripken commented Nov 21, 2023

are you saying that once a large allocation fails a small allocation might also fail?

Yes, exactly. If the browser tab OOMs - "Aw, snap" page - then our test harness is dead in the water and everything after it is lost. At least at some point in the past I saw failed allocations lead to such OOMs, though in principle you'd hope they'd be JS exceptions instead. A long time ago someone explained to me that in Firefox it's very hard to make them all JS exceptions for technical reasons, since a failed allocation could happen in so many different places.

br0nk pushed a commit to br0nk/emscripten that referenced this pull request Nov 29, 2023
…ore#20761)

- remove unneeded `require_wasm64` hack (the default node version we
  ship now supports wasm64 so it is no longer needed)
- remove unneeded `requires_v8` from high memory tests
- remove unneeded `@no_wasm64` from test_4gb_fail
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