-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
servodriver: Shut down Servo elegantly when all tests finish #40455
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
Conversation
|
🔨 Triggering try run (#19134282694) for Linux (WPT) |
|
🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#55907) with upstreamable changes. |
|
✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#55907) title and body. |
1 similar comment
|
✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#55907) title and body. |
|
Test results for linux-wpt from try job (#19134282694): Flaky unexpected result (28)
Stable unexpected results that are known to be intermittent (11)
|
|
I started 6 actions in my fork. It seems 50% of executors can finish, but rest all stuck around 90% indefinitely. I will fix this tomorrow. |
| conn.request("GET", "/status") | ||
| res = conn.getresponse() | ||
| except Exception: | ||
| self.logger.info("Servo has shutted down normally.") |
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.
| self.logger.info("Servo has shutted down normally.") | |
| self.logger.info("Servo has shut down normally.") |
Should this perhaps also be debug? info seems very verbose.
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.
Yeah. Do you know how can I make things appear in console, if I use debug?
| self.logger.info(f"Got response status for shutdown command: {res.status}") | ||
| # 0.05 is a heuristic value manually tested, after which servo always shutted down. | ||
| time.sleep(0.05) | ||
| while self.is_alive(): |
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.
do we perhaps want a timeout here after a couple of tries?
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.
You mean kill the process? Will that result in empty coverage report?
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.
it will result in a missing profile (so the coverage would be missing information from that process / test-run), but better than hanging forever. We could print a warning, if the regular shutdown failed.
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.
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.
|
🔨 Triggering try run (#19155439487) for Linux (WPT) |
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55907). |
|
Test results for linux-wpt from try job (#19155439487): Flaky unexpected result (31)
Stable unexpected results that are known to be intermittent (16)
|
|
🔨 Triggering try run (#19156647195) for Linux (WPT) |
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55907). |
|
Test results for linux-wpt from try job (#19156647195): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (8)
|
|
🔨 Triggering try run (#19158949680) for Linux (WPT) |
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55907). |
|
Test results for linux-wpt from try job (#19158949680): Flaky unexpected result (43)
Stable unexpected results that are known to be intermittent (22)
|
I still don't know why.. This seems so magical. Now 95% can finish; remaining 5% executor stuck at 90%... Will check log later. |
|
✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#55907) title and body. |
|
🔨 Triggering try run (#19319611504) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19319611504): Flaky unexpected result (42)
Stable unexpected results that are known to be intermittent (31)
|
|
✨ Try run (#19319611504) succeeded. |
|
@yezhizhen Is this ready for review? |
yes. |
mrobinson
left a comment
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.
Thanks for doing this!
tests/wpt/tests/tools/wptrunner/wptrunner/browsers/servodriver.py
Outdated
Show resolved
Hide resolved
tests/wpt/tests/tools/wptrunner/wptrunner/browsers/servodriver.py
Outdated
Show resolved
Hide resolved
tests/wpt/tests/tools/wptrunner/wptrunner/browsers/servodriver.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
- Add retry Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
d3ad52c to
f97b92f
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55907). |
Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
f97b92f to
1b4fdec
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55907). |
|
⛔ Failed to properly merge the upstream pull request (web-platform-tests/wpt#55907). Please address any CI issues and try to merge manually. |

Previously, we always kill the instance when tests finish instead of normally shutting down. That blocked clean-ups works, such as Code Coverage we wanted to add, and other things I'm not aware of.
Testing: We have tested for a week and checked the logs of CI to confirm it works normally now.