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.

Sign up for GitHub

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

Jump to bottom

Conduct Exhaustive Testing in CI #3781

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conduct Exhaustive Testing in CI #3781

wants to merge 5 commits into from

Conversation

jginsburgn
Copy link
Member

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.

@jginsburgn jginsburgn force-pushed the exhaustive-tests branch 7 times, most recently from 9250bac to ba35965 Compare March 29, 2022 04:44
jginsburgn pushed a commit that referenced this pull request Mar 29, 2022
refactor: revert #3780
e637e30 
* 263a870 refactor: replace deprecated String.prototype.substr()
* 1b6ded5 refactor: replace .substring() with .slice()
* d6359a7 refactor: replace deprecated String.prototype.substr()

#3780 introduced errors that were
only visible after merging. This should not happen again due to
#3781.
refactor: revert #3780
eb20338 
* 263a870 refactor: replace deprecated String.prototype.substr()
* 1b6ded5 refactor: replace .substring() with .slice()
* d6359a7 refactor: replace deprecated String.prototype.substr()

#3780 introduced errors that were
only visible after merging. This should not happen again due to
#3781.
@jginsburgn jginsburgn requested review from devoto13 and mtrea March 29, 2022 05:27
@devoto13
Copy link
Collaborator

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
Copy link
Collaborator

mtrea commented Mar 29, 2022

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
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devoto13 devoto13 Awaiting requested review from devoto13

@mtrea mtrea Awaiting requested review from mtrea

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jginsburgn @devoto13 @mtrea