-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix strict test suite #24431
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
Fix strict test suite #24431
Conversation
… 'test/runner strict' tests.
9775c08
to
ff33a9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % concern about INCOMING_MODULE_JS_API additive-ness
@@ -1336,17 +1336,17 @@ def in_dir(self, *pathelems): | |||
def add_pre_run(self, code): | |||
assert not self.get_setting('MINIMAL_RUNTIME') | |||
create_file('prerun.js', 'Module.preRun = function() { %s }\n' % code) | |||
self.emcc_args += ['--pre-js', 'prerun.js'] | |||
self.emcc_args += ['--pre-js', 'prerun.js', '-sINCOMING_MODULE_JS_API=[preRun]'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with doing this here is that then the test doesn't have any way to add more to INCOMING_MODULE_JS_API
.
We currently don't have any way to make settings like INCOMING_MODULE_JS_API
additive.
I've tried to make them additive by default (which seems like the right behaviour in almost all cases I can think of): #19938. But that PR got hung up with disagreement about how / if to change the default behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with doing this here is that then the test doesn't have any way to add more to
INCOMING_MODULE_JS_API
.
Yeah, I was thinking about the additivity as well while writing this. That should definitely be the best behavior to have.
The add_pre_run()
is only for tests, and so far it seems that no test needed to concatenate (or that would have shown up in strict
run). If/when such a test comes up, that directive can be refactored to occur at the call site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, either that specific test can then be written to duplicate the -sINCOMING_MODULE_JS_API=[preRun,theothersymbol]
, or -sINCOMING_MODULE_JS_API=[preRun]
be hoisted from add_pre_run()
to all the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM if the tests pass
For errors like |
@@ -9414,7 +9414,11 @@ def test_pthread_dylink_longjmp(self): | |||
@needs_dylink | |||
@node_pthreads | |||
def test_pthread_dylink_main_module_1(self): | |||
self.emcc_args += ['-Wno-experimental', '-pthread', '-lhtml5'] | |||
# TODO: For some reason, -lhtml5 must be passed in -sSTRICT mode, but can NOT | |||
# be passed when not compiling in -sSTRICT mode. That does not seem intentional? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This certainly sounds like a bug. Could you open one an link it here (and below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable
test/runner strict
to pass (tested on Windows)