Rearrange jasmine test retries #3667

archmoj · 2019-03-23T05:13:40Z

A follow up of #3661 and #3599 in order to have more robust jasmine tests with less retries

set shards limit of non-@gl tests to 1 which helped fix various side effects between different tests
removed noCI tags from a number of tests such as gl2d lasso/select & parcoods.
adjusted timeouts of some tests to run on CI
@plotly/plotly_js

reduced number of retries for jasmine-2 reduced number of retries for jasmine-3 adjusted timeouts to help fix test errors on CI removed noCI tags from a number of tests set shards limit to 1 - fixed various side effects between different tests reduce n it limit to reduce side effects between different tests now we can reduce retries for both now test if we could remove some noCI flags from the tests reset few noCI flags final considerations

reset timeout and reduced delays set gl retry to 3

etpinard · 2019-03-25T13:29:25Z

tasks/shard_jasmine_tests.js

@@ -15,7 +15,7 @@ var argv = minimist(process.argv.slice(2), {
        limit: ['l'],
    },
    default: {
-        limit: 20
+        limit: 1


So each test suite is runned separately?

Yes. That's exactly what I wanted there for gl and flaky. I noticed e.g. setting timeout in one suite may have an effect on another suite. After having this in place the tests started to pass properly.

Can you leave to default to 20 and pass a --limit 1 in the relevant test commands in .circleci/test.sh.

Do all npm run test-jasmine commands need to have --limit 1?

etpinard · 2019-03-25T13:36:59Z

@archmoj I'm a little puzzled by this PR. If it makes our tests on CI less flaky than great, but I'd like to why and how much less flaky they get.

applied different retry numbers for gl and flaky containers
reduced number of retries for jasmine-2 test
reduced number of retries for jasmine-3 test

Why does this help?

set shards limit to 1 i.e. helped fix various side effects between different tests

That sounds like an overkill to me. For example, if our tests are less flaky, but take 5x longer to run, this won't help us much. So yeah I'm asking you: after say CI 20 runs, are our tests 2x, 5x, 10x less flaky? How much slower are they?

removed noCI tags from a number of tests such as gl2d lasso/select & parcoods.

🎉 nice!

adjusted timeouts of some tests to run on CI
@plotly/plotly_js

👌

All in all, #3634 sounds more promising. It might be a good idea to wait until @antoinerg is done with that one before spending more time trying to monkey-patch our tests.

…in-jasmine

archmoj · 2019-03-29T03:05:27Z

@etpinard it seems more flaky tests are passing in the first run. The timing are not too bad. And now with @antoinerg great PR #3634 being merged we have:

etpinard · 2019-03-29T13:56:51Z

it seems more flaky tests are passing in the first run

Cool. Can you re-run the tests 10 times and see what's the success rate?

…lly and on the CI set timeout for the rest of the tests in one suite added one noCI flag to the sankey test which is failing on the CI and even locally with flaky flag set flaky retry to 5 now that it runs fast

…in-jasmine

archmoj · 2019-04-01T16:57:35Z

After 10 runs the success rate seems to be around 80 percent i.e. including no flaky suite fails.
https://circleci.com/gh/plotly/workflows/plotly.js/tree/reduce-num-retries-in-jasmine

etpinard · 2019-04-01T17:34:07Z

After 10 runs the success rate seems to be around 80 percent

Is that better than on master?

etpinard · 2019-04-01T17:44:04Z

test/jasmine/tests/gl2d_plot_interact_test.js

@@ -20,6 +20,8 @@ function countCanvases() {
    return d3.selectAll('canvas').size();
 }

+jasmine.DEFAULT_TIMEOUT_INTERVAL = 5000;


Does this apply only to this test suite or to all test suites that get bundled?

Good question. I noticed when using shard with high limit number (e.g. 20), changing a timout in one test has an impact on the other tests in another suites. That may be one reason to try to keep them separated. Anyway a better solution may be to have a timeout setup for every suite/describe block to avoid side effects?

Anyway a better solution may be to have a timeout setup for every suite/describe block to avoid side effects?

Yes, that's a better solution.

@etpinard @antoinerg Any default value there to start with? Should they be set for every describe block? Or having them in every suites is only required?

etpinard · 2019-04-01T17:46:06Z

.circleci/test.sh


 log () {
    echo -e "\n$1"
 }

 # inspired by https://unix.stackexchange.com/a/82602
+MAX_AUTO_RETRY=1


Why would we want to lower MAX_AUTO_RETRY?

It looks flaky tests (jasmine3) sometime required more retries. The gl tests on the other hand are slow and may not need that number of retries. Specially if they fail for a good reason, we don't want to wait too long to be notified that the test is actually failed.

The gl tests on the other hand are slow and may not need that number of retries.

Well, I suspect the gl tests are slow (now) because you reduced the number of test per shards to 1.

To me, having MAX_AUTO_RETRY=5 for all retry loops is the best of both world. Sufficient retry attempt, but failing runs that take to long to exit.

archmoj · 2019-04-01T18:06:40Z

After 10 runs the success rate seems to be around 80 percent

Is that better than on master?

Always hard to tell which one is better. On this branch less noCI flags are applied with less retries. I hope that less is more and that we can have even more robust tests.

etpinard · 2019-04-01T18:13:42Z

On this branch less noCI flags are applied with less retries

I agree, there's a lot of good on this branch, no doubt.

Here are my recommendations (to get this thing merged):

Bring back MAX_AUTO_RETRY=5 for all retry loops
Bring back the limit: 20 default in the shard utility
Lower the shard limit for the non-@gl test-jasmine commands
Bump the timeout in gl2d_plot_interact_test.js per it block as opposed to that jasmine.DEFAULT_TIMEOUT_INTERVAL = 5000; line in the file scope

reset shard limit to 20 and retry to 5 revised timout as well as before and after functions in describe blocks in gl2d_plot_interact_test lower the shard limit for the non-gl test-jasmine commands

archmoj · 2019-04-01T19:53:06Z

I agree, there's a lot of good on this branch, no doubt.

Here are my recommendations (to get this thing merged):

Bring back MAX_AUTO_RETRY=5 for all retry loops

Bring back the limit: 20 default in the shard utility

Lower the shard limit for the non-@gl test-jasmine commands

Bump the timeout in gl2d_plot_interact_test.js per it block as opposed to that jasmine.DEFAULT_TIMEOUT_INTERVAL = 5000; line in the file scope

@etpinard Thanks for the recommendations.
Applied in 24aafc8.

etpinard · 2019-04-01T19:54:37Z

Ok, let's get this in 💃

archmoj added type: maintenance and removed status: reviewable labels Mar 23, 2019

moved gl2d double-click into another file

c57d316

reset timeout and reduced delays set gl retry to 3

archmoj added status: reviewable and removed status: in progress labels Mar 24, 2019

etpinard reviewed Mar 25, 2019

View reviewed changes

archmoj added 3 commits March 28, 2019 20:59

Merge remote-tracking branch 'origin/master' into reduce-num-retries-…

5395282

…in-jasmine

reduced retries to 2 and 3 - marked one test as flaky

8241bc8

no foreach may help run the CI

230544f

archmoj added 3 commits March 29, 2019 16:39

should reset one sankey test - no need to flaky flag as it fails loca…

17b5068

…lly and on the CI set timeout for the rest of the tests in one suite added one noCI flag to the sankey test which is failing on the CI and even locally with flaky flag set flaky retry to 5 now that it runs fast

Merge remote-tracking branch 'origin/master' into reduce-num-retries-…

4b0b824

…in-jasmine

set flaky retry number to 4 - before rerun 10 times

305e0b3

etpinard reviewed Apr 1, 2019

View reviewed changes

Apply Etienne recommendations

24aafc8

reset shard limit to 20 and retry to 5 revised timout as well as before and after functions in describe blocks in gl2d_plot_interact_test lower the shard limit for the non-gl test-jasmine commands

archmoj merged commit c657348 into master Apr 1, 2019

archmoj deleted the reduce-num-retries-in-jasmine branch April 1, 2019 19:55

archmoj mentioned this pull request Apr 10, 2019

More stable jasmine2 test runs #3756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rearrange jasmine test retries #3667

Rearrange jasmine test retries #3667

archmoj commented Mar 23, 2019 •

edited

Loading

etpinard Mar 25, 2019

archmoj Mar 25, 2019

etpinard Apr 1, 2019

etpinard commented Mar 25, 2019

archmoj commented Mar 29, 2019

etpinard commented Mar 29, 2019

archmoj commented Apr 1, 2019

etpinard commented Apr 1, 2019

etpinard Apr 1, 2019 •

edited

Loading

archmoj Apr 1, 2019

etpinard Apr 1, 2019

archmoj Apr 1, 2019

etpinard Apr 1, 2019

archmoj Apr 1, 2019

etpinard Apr 1, 2019

archmoj commented Apr 1, 2019

etpinard commented Apr 1, 2019

archmoj commented Apr 1, 2019

etpinard commented Apr 1, 2019

Rearrange jasmine test retries #3667

Rearrange jasmine test retries #3667

Conversation

archmoj commented Mar 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Mar 25, 2019

archmoj commented Mar 29, 2019

etpinard commented Mar 29, 2019

archmoj commented Apr 1, 2019

etpinard commented Apr 1, 2019

etpinard Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archmoj commented Apr 1, 2019

etpinard commented Apr 1, 2019

archmoj commented Apr 1, 2019

etpinard commented Apr 1, 2019

archmoj commented Mar 23, 2019 •

edited

Loading

etpinard Apr 1, 2019 •

edited

Loading