Conduct Exhaustive Testing in CI

[jginsburgn]

Since CI does not conduct all tests in all environments (OS and Node.js version) it can miss failures, such as what happened with #3780. This will ensure that is less likely to happen.

[devoto13]

To be honest, I don't quite agree with these changes as I don't see what problem do they solve.

The post-merge is expected to fail sometimes because it runs tests in different browsers which we can't do in the PRs without exposing BrowserStack/SauceLabs credentials. Otherwise, I think the current workflows sufficiently cover different OS and Node versions without wasting resources on redundant combinations.

The failure you refer to looks like a temporary BrowserStack glitch unrelated to #3780 and was resolved by re-running the job. Not sure why the first two attempts failed, but it does not look like it was related to the changes from that PR.

[mtrea]

I think Yaroslav raises a good point. The value of incremental test coverage needs to be considered against the relative cost (maintenance of the coverage + compute resources + more time spent waiting for tests to finish). I'd be interested to hear if you're still interested in pursuing this Jonathan, and if so, why.

[jginsburgn]

I agree with @devoto13 in that the way it is configured now, we cannot conduct pre-merge testing in SauceLabs and BrowserStack. However, I propose a workaround for that in #3786. Let's continue the conversation on this topic there.

On the other point that @devoto13 raises, about our current testing being sufficient in the environment combinations (Node.js versions and OSs), I partially disagree. I encountered that commitlint fails in windows currently, so contributors could not run it if working in that OS. We were not aware of this because we do not conduct linting in Windows currently. So I think adding the combinations is valuable. Also, addressing @mtrea 's comment about computing resources I think we are way below reaching the GitHub actions computing time we have. In regards to waiting for tests, we could parallelize the other combinations of tests.

So, if you both agree, I can repurpose this CL to only expand the environment combinations for our testing workflow and drop making BrowserStack mandatory for now. WDYT @devoto13 and @mtrea ?

[devoto13]

I encountered that commitlint fails in windows currently, so contributors could not run it if working in that OS.

Do you refer to the npm run commitlint -- --from `git merge-base origin/master $GITHUB_SHA` command from the test.yml? This was never intended to run on Windows or used by the contributors as it is specifically designed for the Linux and CI. Contributors on Windows should use npm run check:commit script, which should work just fine. Doesn't it?

My assumption is that contributors would normally use latest LTS Node (not the oldest supported Node) when developing, so we don't really need to test our dev-tools (e.g. npm run lint, npm run build:check, etc) on all supported Node versions as they would produce exactly the same result. However our consumers can run karma on various Node versions, so that's why we want to make sure that unit/e2e tests pass on all supported Node versions/OS's to prevent regressions.

I won't block if you really want to run dev-tools on all Node versions, but I still think that it is wasteful and has little to no value.

[jginsburgn]

Do you refer to the npm run commitlint -- --from git merge-base origin/master $GITHUB_SHA command from the test.yml? This was never intended to run on Windows or used by the contributors as it is specifically designed for the Linux and CI.

Yes. I am referring to that step! Why was it not intended to run on Windows? I thought it would. After all we have specified the workflow to use Bash, even on Windows.

My assumption is that contributors would normally use latest LTS Node (not the oldest supported Node) when developing, so we don't really need to test our dev-tools (e.g. npm run lint, npm run build:check, etc) on all supported Node versions as they would produce exactly the same result. However our consumers can run karma on various Node versions, so that's why we want to make sure that unit/e2e tests pass on all supported Node versions/OS's to prevent regressions.

I agree on your point about the Node.js version; let's only test our dev-tools in the LTS one. However, I also want to ensure that developers on Windows can run them. WDYT @devoto13?

[devoto13]

Yes. I am referring to that step! Why was it not intended to run on Windows? I thought it would. After all we have specified the workflow to use Bash, even on Windows.

Yeah-yeah, I meant cmd when saying Windows. It will indeed work in bash on Windows.

I agree on your point about the Node.js version; let's only test our dev-tools in the LTS one. However, I also want to ensure that developers on Windows can run them. WDYT @devoto13?

Not sure how common it is to use cmd on Windows when developing for Node... But I would not mind having main-windows and main-linux which would run all checks on both OS's. I suspect that test:e2e would not work on Windows with cmd right now.