[test] Kill and restart browser on test failure #20828
Conversation
sbc100
force-pushed
the
browser_restart
branch
2 times, most recently
from
d1ee177 to
1669eb5
Compare
sbc100
force-pushed
the
browser_restart
branch
from
1669eb5 to
c00d8d4
Compare
|
ptal |
sbc100
force-pushed
the
browser_restart
branch
2 times, most recently
from
d01472b to
cde3b9d
Compare
| EMTEST_SKIP_SIMD: "1" | ||
| EMTEST_SKIP_SCONS: "1" | ||
| EMTEST_SKIP_NODE_CANARY: "1" | ||
| EMTEST_BROWSER: "0" |
There was a problem hiding this comment.
Without this line the attempt to run sockets.test_nodejs_sockets_echo would launch a browser (because test_sockets.py is a browser test suite).
There was a problem hiding this comment.
Hmm, but why is this needed now, and not before this PR?
| cls.browser_proc.wait(1) | ||
| except subprocess.TimeoutExpired: | ||
| cls.browser_proc.kill() | ||
| cls.browser_proc.wait() |
There was a problem hiding this comment.
Maybe add a comment about why it makes sense to give it a 1-second chance to exit gracefully? I'm not sure why that is, actually.
There was a problem hiding this comment.
Before this PR we were using python's webbrowser.get(..).open_new(url) which I assume would open the system browser on windows.
After this PR we default to using subprocess.Popen(['google-chrome'...) .. which doesn't work unless you have that browser in the PATH.
However, in this case we don't want to be starting any browser at all.
There was a problem hiding this comment.
Sorry, I'm confused. I asked about the terminate/wait/kill/wait sequence here, but you replied about starting the browser? Are those connected in a way I do not understand?
There was a problem hiding this comment.
Somehow the replies to these comments got mixed up. Here I'm commenting on as to why the EMTEST_BROWSER=0 is needed when running the socket tests on windows.
There was a problem hiding this comment.
Regarding the using of terminate() prior to kill() I added comment.
It just seemed sensible to me to give the browser a chance to exit cleanly before killing with fire.
sbc100
force-pushed
the
browser_restart
branch
from
cde3b9d to
46fbcbb
Compare
2 participants
Fixes: #20820