Cleanup the emscripten tests relating to the event loop. NFC #23760
Conversation
sbc100
force-pushed
the
event_loop_tests
branch
2 times, most recently
from
f2af566 to
6970efe
Compare
sbc100
changed the title
sbc100
force-pushed
the
event_loop_tests
branch
3 times, most recently
from
295f2fc to
c1a2f16
Compare
The browser tests were only being run in `PROXY_TO_PTHREAD` mode for some reason even though these tests should pass on the main thread. In test_other the `PROXY_TO_PTHREAD` flags were being passed as args (rather then emcc_args). Likely a copy/paste bug.
sbc100
force-pushed
the
event_loop_tests
branch
from
c1a2f16 to
a6b1367
Compare
sbc100
changed the title
| self.btest_exit('gl_textures.c', args=['-lGL', '-g', '-sSTACK_SIZE=1MB'] + args) | ||
| @also_with_proxy_to_pthread | ||
| def test_gl_textures(self): | ||
| self.btest_exit('gl_textures.c', args=['-lGL', '-g', '-sSTACK_SIZE=1MB', '-sOFFSCREEN_FRAMEBUFFER']) |
There was a problem hiding this comment.
This did not always test with OFFSCREEN_FRAMEBUFFER before. Do we not error on that being used without pthreads?
There was a problem hiding this comment.
No, I think think its harmlessly ignored.
There was a problem hiding this comment.
Maybe we should error ? But we don't today.
| def test_emscripten_set_interval(self): | ||
| self.do_runf('emscripten_set_interval.c', args=['-pthread', '-sPROXY_TO_PTHREAD']) | ||
| self.do_runf('emscripten_set_interval.c') |
There was a problem hiding this comment.
Previously this was tested with pthreads, but now it isn't, if I read this diff correctly?
There was a problem hiding this comment.
No, see the PR description. There is a typo here. args should be emcc_args so this was never working here. args in this context refers to arguments for the program itself.
There was a problem hiding this comment.
I see. Should we fix the typo and test it with pthreads, or not?
There was a problem hiding this comment.
I can't think of a reason to use pthreads on this myself. lgtm either way.
There was a problem hiding this comment.
We test with and without threads in the browser already.
| def test_emscripten_set_interval(self): | ||
| self.do_runf('emscripten_set_interval.c', args=['-pthread', '-sPROXY_TO_PTHREAD']) | ||
| self.do_runf('emscripten_set_interval.c') |
There was a problem hiding this comment.
I can't think of a reason to use pthreads on this myself. lgtm either way.
The browser tests were only being run in
PROXY_TO_PTHREADmodefor some reason even though these tests should pass on the main thread.
In test_other the
PROXY_TO_PTHREADflags were being passed as args(rather then emcc_args). Likely a copy/paste bug.