🏗 Pass CircleCI matrix parameters to (module|nomodule)-tests.js as a command line flag

[danielrozenberg]

As discussed in #32865 (comment)

[amp-owners-bot]

Hey @rsimha! These files were changed:

.circleci/config.yml
build-system/pr-check/module-tests.js
build-system/pr-check/nomodule-tests.js

[rsimha]

LGTM! Thanks for the quick fix.

[rsimha]

@danielrozenberg Apologies in advance for what is going to be a disruptive comment to this PR:

Your exploration in #32932 and the subsequent points in #32932 (comment) make me believe we should un-split the Nomodule / Module Tests jobs back to single jobs that run both prod and canary tests one after another, using the new stuff in #32927.

The actual running of integration tests takes <2 minutes, so the full cycle of downloading binaries, running prod tests, applying the canary config, and running canary tests will in total still take less time than the E2E Tests job which runs in parallel, so this won't affect the total wait time for a CI build.

Yet, it will reduce the number of VMs blocked by a single build, and will allow for more people's jobs to run simultaneously.

WDYT?

[danielrozenberg]

I dunno, it doesn't feel right to have both of them run in the same job - my thinking is that each test suite should run separately, so that a failure in one wouldn't preclude showing the results of the following test suite. i.e., if we run gulp integration --prod and afterwards gulp integration --canary, but the prod tests fail, the developer won't know that the test also fails (or maybe not) on canary

I think the clarity of the results overrides the slight overrun

[rcebulko]

CircleCI already is a decent improvement in speed; are we seeing a shortage of VMs slowing down development during non-Fixit weeks?

[danielrozenberg]

CircleCI already is a decent improvement in speed; are we seeing a shortage of VMs slowing down development during non-Fixit weeks?

@rsimha's worry is about the max parallelization - we get 80 parallel VMs, so increasing PR runs from 18 to 20 VMs each means we can check 4 instead of 4.44 PRs in parallel, which is a small but not insignificant drop

[rsimha]

we get 80 parallel VMs

... for the entire ampproject org, not just for amphtml.

my thinking is that each test suite should run separately, so that a failure in one wouldn't preclude showing the results of the following test suite

We already preclude later results when one result fails in a lot of places. E.g. if presubmit fails, we don't run lint. If unit tests fail for local changes, we don't run all unit tests. If unit tests fail on Safari, FF, or Edge, we don't run integration or E2E tests. In the case of integration tests for prod vs. canary, we could make a case to apply the same rationale.

worry is about the max parallelization

Correct, this is my main concern. We should draw a balance between finishing a build ASAP and allowing as many people's builds to run in parallel.

In addition, being judicious about resource usage saves us $$$, because one extra minute of CPU time per CI job multiplies out to nearly half a million extra minutes over a year.

Having said all this, I'm okay merging this PR as-is and assessing these concerns separately since things aren't as bad yet. Also, thanks for the discussions, because they're helping keep me honest 😃

[danielrozenberg]

As you can already guess I prefer to merge this as-is ;)

Let's keep checking the pulse as things evolve