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

Add Repeat and Retry functionality to sigh test #2590

Merged
merged 5 commits into from
Jan 29, 2019
Merged

Add Repeat and Retry functionality to sigh test #2590

merged 5 commits into from
Jan 29, 2019

Conversation

thebrianchen
Copy link

@thebrianchen thebrianchen commented Jan 15, 2019

Adding the option to allow retries and repeat tests. I initially tried adding repeat functionality directly into mocha by encapsulating runner.run() which would allow test pass/fail/end hooks to be added more easily. However, I ran into a mocha implementation issue that requires the entire mocha instance to be re-instantiated and the 'require' cache cleared in between runs.

I removed the return from test() -- not sure where it's being used.

Also, not sure if the package.lock should be excluded // how to auto-exclude it from future commits.

@thebrianchen thebrianchen requested a review from lindner January 15, 2019 01:25
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@thebrianchen thebrianchen reopened this Jan 15, 2019
@thebrianchen thebrianchen self-assigned this Jan 15, 2019
@thebrianchen
Copy link
Author

I signed it!

tools/sigh.js Outdated Show resolved Hide resolved
tools/sigh.js Outdated Show resolved Hide resolved
devtools/package-lock.json Outdated Show resolved Hide resolved
@lindner lindner changed the title Repeat and retry functionality Add Repeat and Retry functionality to sigh test Jan 15, 2019
@googlebot
Copy link

CLAs look good, thanks!

@lindner
Copy link
Contributor

lindner commented Jan 15, 2019

Check the return-code you're generating? If any test fails you should return non-zero for the test phase.

😱
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] test-sigh: `cross-env tools/sigh`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] test-sigh script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2019-01-15T21_18_55_679Z-debug.log

tools/sigh.js Outdated Show resolved Hide resolved
@lindner lindner requested a review from raulverag January 22, 2019 23:43
@lindner
Copy link
Contributor

lindner commented Jan 22, 2019

LGTM, Raul for approval.

@raulverag
Copy link
Collaborator

I don't see why we would want to repeat or retry tests within one run. What does it even mean if the tests pass on one repeat and then fail on another, other than that the tests are flaky? Furthermore, it seems that if the last run succeeds, the user won't even see any failing output, because that will have scrolled off the screen. Correct me if I'm mistaken, and please elaborate as to what purpose repeats and retries serve.

@shans
Copy link
Contributor

shans commented Jan 23, 2019

Friendly reminder to please discuss changes (such as this) which have the potential to change development practices for all of us in a team meeting, ideally before starting development.

@lindner
Copy link
Contributor

lindner commented Jan 23, 2019

For folks tuning in, this work was done to specifically address issue #779

My apologies for not bringing this up in a larger context. It was deemed something simple enough as a good-first-issue.

@lindner
Copy link
Contributor

lindner commented Jan 23, 2019

Raul suggested that this needs to have summary output at the end.

@raulverag
Copy link
Collaborator

Yes. If the idea is to help diagnose flaky tests, as described in #779, then the results must be displayed in some kind of summary, e.g. "x out of y tests succeeded. First failure was repeat N.", or in the case of retries, "Tests succeeded after X retries".

@thebrianchen
Copy link
Author

The mocha.js implementation of retries does not allow for logging and will simply run the test until it either passes or the retry count is exhausted, so I removed the the retries flag option.

Added a summary to the end of the runs. Example output:

100 runs completed. 4 runs failed.
Failed runs: [4, 47, 49, 81]

@lindner
Copy link
Contributor

lindner commented Jan 28, 2019

LGTM, Raul for final approvals.

@thebrianchen thebrianchen merged commit 9295cb3 into PolymerLabs:master Jan 29, 2019
@thebrianchen thebrianchen deleted the repeat_flag branch January 29, 2019 00:40
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.

5 participants