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

🏗️ enable 3-at-a-time batching for all sauce labs tests #27051

Merged
merged 9 commits into from Mar 11, 2020

Conversation

rcebulko
Copy link
Contributor

@rcebulko rcebulko commented Mar 2, 2020

Comparing with #27016 to see batch size impact on Travis running time

@amp-owners-bot
Copy link

amp-owners-bot bot commented Mar 2, 2020

Hey @estherkim, these files were changed:

build-system/pr-check/remote-tests.js
build-system/tasks/runtime-test/helpers.js

@rcebulko
Copy link
Contributor Author

rcebulko commented Mar 3, 2020

For context/discussion/analysis of the ideal batch size, see #27016

rsimha
rsimha previously requested changes Mar 3, 2020
Copy link
Contributor

@rsimha rsimha 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 the exploration and for putting together this PR. I'm adding a blocking review because this PR as written will break CI builds. Suggestions to fix are in the comments below.

build-system/pr-check/remote-tests.js Outdated Show resolved Hide resolved
build-system/tasks/runtime-test/helpers.js Outdated Show resolved Hide resolved
@rcebulko
Copy link
Contributor Author

rcebulko commented Mar 3, 2020

Not to be addressed at this moment, but something I noticed when looking at the batching code. It doesn't continuously run "up to $BATCHSIZE batches". That is, when it runs 6 tests with a batch size of 3, it waits until all 3 complete before triggering another 3 at once. Instead of running in batches like this, we could probably speed up runtime by using a pool instead. This way, as soon as one browser finishes, another can start.

@rcebulko
Copy link
Contributor Author

rcebulko commented Mar 3, 2020

It appears we also re-transform tests with browserify once each batch; I'm guessing we should be able to do that once for all the browsers, stable and beta, instead of doing it per-batch. Each browserify invocation takes a minute or so.

@rcebulko rcebulko requested a review from rsimha March 3, 2020 22:23
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

Thanks! Do you know why we're testing on Chrome twice? https://travis-ci.org/ampproject/amphtml/jobs/657948850#L2400-L2402

Also, can you update the comments here

* If --stable is provided, runs tests only on stable browsers in sauce labs without batching.
* If --beta is provided, runs tests only on beta browsers in sauce labs without batching. Does not fail.
* If neither --stable nor --beta are provided, runs test on all browsers in sauce labs with batching.

build-system/tasks/runtime-test/helpers.js Outdated Show resolved Hide resolved
@rsimha rsimha dismissed their stale review March 5, 2020 21:51

Withdrawing my blocking review now that test status reporting is no longer broken. Will defer to @estherkim for final approval of the code changes. LGTM from my side.

Comment on lines 262 to 263
beta: argv.stable ? [] : config.browsers.filter(isBeta),
stable: argv.beta ? [] : config.browsers.filter(isStable),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we flip this so that stable browsers are set when argv.stable is set? And if neither flags are present, default to all or stable, up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, PTAL

Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants