Skip to content

Auto-retry jasmine test on CI #2373

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

Merged
merged 2 commits into from
Feb 16, 2018
Merged

Auto-retry jasmine test on CI #2373

merged 2 commits into from
Feb 16, 2018

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Feb 15, 2018

Splitting the jasmine tests into two CI jobs did help alleviate some of our intermittent test failures, but these still happen in about 1 run out of 2.

As Circle CI 2.0 doesn't have an auto-retry setting yet, we currently have to painfully navigate the CircleCI UI to restart our failed jobs. This is of course a waste time. So, I propose this in-house solution.

@alexcjohnson @dfcreative what do you think?

@etpinard
Copy link
Contributor Author

See example here where the first npm run test-jasmine call failed, but the second succeeded.

@alexcjohnson
Copy link
Collaborator

Mostly I love it, but I'm a little concerned about two things:

  • If there really is a test failure you didn't notice locally, it will take 5 times as long before you see it
  • At some point we should really fix these tests so they are robust, rather than just continuing to throw extra computing power at them. This solution hides the failures so it'll be hard to figure out where they are.

There was some discussion about how this could be done within jasmine, to just retry failing tests, they left it as the not very helpful "I think you should be able to build something that can do this with the existing custom reporter and filter function" but if we could get this to work that would be the best of all jasmine/jasmine#960

One idea though that I think would satisfy my concerns: can we make a tag for known unreliable tests, split those out into a separate task, and only run those ones in the retry loop? That way we would have a place to look to see which tests these are (and fix them when we get a chance), and other test failures wouldn't get slowed down.

@etpinard
Copy link
Contributor Author

Mostly I love it

I'll take it. I was expecting this PR to be somewhat controversial. 😌


A few comments on your comments:

If there really is a test failure you didn't notice locally, it will take 5 times as long before you see it

That's not 100% true, one could still open up the CircleCI test page and stop the run when a true failure occurs. But yeah, this isn't ideal.

At some point we should really fix these tests so they are robust, rather than just continuing to throw extra computing power at them. This solution hides the failures so it'll be hard to figure out where they are.

Of course, that would be best. But as making tests robust on CI is very time consuming and sometimes frustrating, I hope this auto-retry solution is a step in the right direction.

There was some discussion about how this could be done within jasmine, to just retry failing tests

That would decrease our average total test time, but that may hide failures that occur because of bad test teardown. For example, if two tests ran in succession count the number of some DOM node and the first doesn't call destroyGraphDiv after completion, the second might fail in a full run, and pass in an isolated run.

One idea though that I think would satisfy my concerns: can we make a tag for known unreliable tests, split those out into a separate task, and only run those ones in the retry loop?

I like this solution too. I'm thinking of adding @flaky spec tag. Commit coming soon.

@@ -720,7 +720,7 @@ describe('Animating multiple axes', function() {
destroyGraphDiv();
});

it('updates ranges of secondary axes', function(done) {
it('@flaky updates ranges of secondary axes', function(done) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you think of any other flaky tests worth adding to the list @alexcjohnson ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure, I think you got the most common ones, but now it's super easy to add @flaky if another test fails spuriously. Lets just keep an eye out for failures over the next few days after merging this.

@alexcjohnson
Copy link
Collaborator

Lovely, this should really help our productivity! 💃

@dy
Copy link
Contributor

dy commented Feb 16, 2018

Brilliant solution @etpinard, thanks

@etpinard etpinard merged commit 42a7c77 into master Feb 16, 2018
@etpinard etpinard deleted the ci-retry branch February 16, 2018 18:51
@etpinard
Copy link
Contributor Author

Flaky test alert:

image

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