Skip to content

Conversation

@brendandahl
Copy link
Collaborator

No description provided.

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.

Nice!

@brendandahl brendandahl force-pushed the jspi-browser-test branch 2 times, most recently from dd78012 to e4c2c19 Compare March 22, 2023 17:01
@kripken
Copy link
Member

kripken commented Mar 22, 2023

Could we enable one of the existing tests in JSPI mode perhaps? (say, test_async or test_wget) Or do those need more than JSPI supports atm?

@brendandahl
Copy link
Collaborator Author

Could we enable one of the existing tests in JSPI mode perhaps? (say, test_async or test_wget) Or do those need more than JSPI supports atm?

Good point. I originally thought I needed a more thorough test because test_async was passing without JSPI enabled, but it turns out my chrome profile already had JSPI enabled. I switched to a new profile now for tests now.

@parameterized({
'asyncify': (['-sASYNCIFY=1'],),
'jspi': (['-sASYNCIFY=2', '-Wno-experimental'],),
})
Copy link
Member

Choose a reason for hiding this comment

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

If we add more of these it might make sense to add a @also_with_jspi decorator or such, like we have for other stuff, but I'm not sure. Just a thought for later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I was going to add this, but wasn't sure what happens when you have multiple -sASYNCIFY args i.e. the main test will have -sASYNCIFY=1, but the @also_with_jspi will add -sASYNCIFY=2. I'm guessing the last overwrites it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, later -s flags should override.

@brendandahl brendandahl merged commit b116323 into emscripten-core:main Mar 23, 2023
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.

3 participants