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.

Sign up for GitHub

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

Jump to bottom

[Reporting] Fix browser diagnostic Jest test flakiness #118003

Merged
merged 8 commits into from
Nov 11, 2021
Merged

[Reporting] Fix browser diagnostic Jest test flakiness #118003

merged 8 commits into from
Nov 11, 2021

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Nov 9, 2021 •
edited
Loading

Summary

Closes #89369
Closes #89200

The proposed change is to reduce the amount of time tests need to wait for the mocked browser logs to be collected. This will move the timing away from the, currently, dangerously close 6 second timeout.

Tests

Test timings before the change

 PASS  x-pack/plugins/reporting/server/routes/diagnostic/browser.test.ts (45.556 s)
  POST /diagnose/browser
    ✓ returns a 200 when successful (5135 ms)
    ✓ returns logs when browser crashes + helpful links (5066 ms)
    ✓ logs a message when the browser starts, but then has problems later (5055 ms)
    ✓ logs a message when the browser starts, but then crashes (5050 ms)
    ✓ cleans up process and subscribers (5042 ms)

Test timings after the change (timeout set to 50ms)

 PASS  x-pack/plugins/reporting/server/routes/diagnostic/browser.test.ts (25.754 s)
  POST /diagnose/browser
    ✓ returns a 200 when successful (196 ms)
    ✓ returns logs when browser crashes + helpful links (101 ms)
    ✓ logs a message when the browser starts, but then has problems later (109 ms)
    ✓ logs a message when the browser starts, but then crashes (90 ms)
    ✓ cleans up process and subscribers (101 ms)

Notes & TODO

@jloleysens jloleysens added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Reporting Services v8.1.0 v7.16.1 labels Nov 9, 2021
@jloleysens jloleysens requested review from a team as code owners November 9, 2021 11:42
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

dokmic
dokmic reviewed Nov 9, 2021
View reviewed changes
x-pack/plugins/reporting/server/browsers/chromium/driver_factory/constants.ts Outdated

const BROWSER_LAUNCH_TIME_TO_WAIT = 5 * 1000;

export const getBrowserLaunchTime = () => BROWSER_LAUNCH_TIME_TO_WAIT;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the chosen approach is correct. It is not a good idea to have a helper function returning a constant just for unit tests.
Have you tried jest.useFakeTimers?
If that doesn't work, then the second option will be using RxJS marbles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll look into that 👍🏻

dokmic
dokmic approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@dokmic dokmic left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback! LGTM 👍

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jloleysens jloleysens merged commit 532610b into elastic:main Nov 11, 2021
@jloleysens jloleysens deleted the reporting/fix-diagnose-jest-test branch November 11, 2021 12:17
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 11, 2021
@jloleysens @kibanamachine
[Reporting] Fix browser diagnostic Jest test flakiness (elastic#118003)
b2df90b 
* mock and reduce the wait time for reading diagnoistic logs

* remove comment

* run test suite 42 times - REVERT THIS

* Revert "run test suite 42 times - REVERT THIS"

This reverts commit f9474aa.

* mock Rx.timer instead, revert previous solution

* added comment

* remove for loop

Co-authored-by: Kibana Machine <[email protected]>
@jloleysens jloleysens mentioned this pull request Nov 11, 2021
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 11, 2021
@jloleysens @kibanamachine
[Reporting] Fix browser diagnostic Jest test flakiness (elastic#118003)
4da06ec 
* mock and reduce the wait time for reading diagnoistic logs

* remove comment

* run test suite 42 times - REVERT THIS

* Revert "run test suite 42 times - REVERT THIS"

This reverts commit f9474aa.

* mock Rx.timer instead, revert previous solution

* added comment

* remove for loop

Co-authored-by: Kibana Machine <[email protected]>
@jloleysens jloleysens mentioned this pull request Nov 11, 2021
Merged
jloleysens added a commit that referenced this pull request Nov 11, 2021
@jloleysens @kibanamachine
[Reporting] Fix browser diagnostic Jest test flakiness (#118003) (#11…
79cb334 
…8325)

* mock and reduce the wait time for reading diagnoistic logs

* remove comment

* run test suite 42 times - REVERT THIS

* Revert "run test suite 42 times - REVERT THIS"

This reverts commit f9474aa.

* mock Rx.timer instead, revert previous solution

* added comment

* remove for loop

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
jloleysens added a commit that referenced this pull request Nov 11, 2021
@jloleysens @kibanamachine
[Reporting] Fix browser diagnostic Jest test flakiness (#118003) (#11…
c590b18 
…8326)

* mock and reduce the wait time for reading diagnoistic logs

* remove comment

* run test suite 42 times - REVERT THIS

* Revert "run test suite 42 times - REVERT THIS"

This reverts commit f9474aa.

* mock Rx.timer instead, revert previous solution

* added comment

* remove for loop

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
This was referenced Nov 22, 2021
Closed
Closed
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
@jloleysens @kibanamachine @dmlemeshko
[Reporting] Fix browser diagnostic Jest test flakiness (#118003)
cae965a 
* mock and reduce the wait time for reading diagnoistic logs

* remove comment

* run test suite 42 times - REVERT THIS

* Revert "run test suite 42 times - REVERT THIS"

This reverts commit f9474aa.

* mock Rx.timer instead, revert previous solution

* added comment

* remove for loop

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dokmic dokmic dokmic approved these changes

@tsullivan tsullivan Awaiting requested review from tsullivan

@spalger spalger Awaiting requested review from spalger

Assignees
No one assigned
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.16.1 v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
4 participants
@jloleysens @elasticmachine @kibanamachine @dokmic