Skip to content
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

βœ…πŸ‘· kill browserstack execution early #3096

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

e2e-bs tests are still quite flaky, sometimes because of a BrowserStack issue (?). Because we can run a single e2e-bs job at a time, we should detect and stop the jobs as soon as possible so other can be run.

Changes

When detecting an unrecoverable error in the command output, or if the command doesn't print anything within 5 minutes, let's kill it early, we don't wait for nothing.

For now, we don't implement a retry mechanism, but it could be done later.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

When detecting an unrecoverable error in the command output, or if the
command doesn't print anything within 5 minutes, let's kill it early, we
don't wait for nothing.
@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner October 25, 2024 14:48
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 93.17%. Comparing base (2752f59) to head (4c7c6fe).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3096      +/-   ##
==========================================
- Coverage   93.66%   93.17%   -0.49%     
==========================================
  Files         276      276              
  Lines        7624     7624              
  Branches     1710     1710              
==========================================
- Hits         7141     7104      -37     
- Misses        483      520      +37     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

}

clearTimeout(timeoutId)
setTimeout(() => killIt('no output timeout'), NO_OUTPUT_TIMEOUT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

πŸ”¨ warning: ‏

Suggested change
setTimeout(() => killIt('no output timeout'), NO_OUTPUT_TIMEOUT)
timeoutId = setTimeout(() => killIt('no output timeout'), NO_OUTPUT_TIMEOUT)

Copy link
Member Author

Choose a reason for hiding this comment

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

Aah good catch, that's why it didn't work :D

}

function killIt(message) {
if (!isKilled) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏ Do we need this isKilled variable? Instead, can we return on line 74? (unless child.kill('SIGTERM') does not terminate immediately and somehow some more output is processed.

Copy link
Member Author

Choose a reason for hiding this comment

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

unless child.kill('SIGTERM') does not terminate immediately and somehow some more output is processed

"SIGTERM" is typically a "graceful exit" -> the process can catch it and continue its execution to clean things up before exiting. So it is possible to have some output after a SIGTERM.

In practice, I'm not sure it happens in this particular situation (= karma and wdio processes). Sending the signal multiple times shouldn't hurt, so let's simplify the code!

Copy link

Bundles Sizes Evolution

πŸ“¦ Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 161.23 KiB 161.23 KiB 0 B 0.00% βœ…
Logs 56.00 KiB 56.00 KiB 0 B 0.00% βœ…
Rum Slim 109.80 KiB 109.80 KiB 0 B 0.00% βœ…
Worker 25.21 KiB 25.21 KiB 0 B 0.00% βœ…
πŸš€ CPU Performance
Action Name Base Average Cpu Time (ms) Local Average Cpu Time (ms) 𝚫
addglobalcontext 0.002 0.002 -0.001
addaction 0.057 0.033 -0.023
addtiming 0.001 0.001 -0.000
adderror 0.055 0.037 -0.018
startstopsessionreplayrecording 1.366 0.970 -0.396
startview 1.577 1.203 -0.374
logmessage 0.031 0.019 -0.012
🧠 Memory Performance
Action Name Base Consumption Memory (bytes) Local Consumption Memory (bytes) 𝚫 (bytes)
addglobalcontext 7.82 KiB 7.53 KiB -299 B
addaction 38.63 KiB 38.54 KiB -97 B
addtiming 6.95 KiB 5.89 KiB -1075 B
adderror 43.35 KiB 42.54 KiB -831 B
startstopsessionreplayrecording 3.53 KiB 4.71 KiB 1.18 KiB
startview 460.40 KiB 466.63 KiB 6.23 KiB
logmessage 38.80 KiB 39.83 KiB 1.04 KiB

πŸ”— RealWorld

@BenoitZugmeyer BenoitZugmeyer merged commit 22d5c6f into main Oct 31, 2024
19 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/bs-early-failure branch October 31, 2024 17:27
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