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

Limit test utils to a single environment (browser / ssr). #195

Merged
merged 11 commits into from
Apr 19, 2021
Merged

Conversation

MMMalik
Copy link
Contributor

@MMMalik MMMalik commented Apr 13, 2021

This PR achieves the following:

  1. Refactors various test utils so that tests are run for a requested browser (or ssr) rather than for the whole permuatation of browsers and React versions. Most notably react-tester script is parametrized and accepts a few arguments (React version, browser, flag for dry run). Config for Karma is changed to use few BrowserStack-specific adjustments.

  2. Travis CI is replaced with GitHub workflows. There are 2 files for GH actions: pr-check is supposed to run on PRs to master only and is supposed to be a quick feedback (runs tests for a single React version and on a single browser). master-check is supposed to run on commits to master and it tests whole permutation of React versions and browsers. As an exception and for the convenience of reviewer, master-check will run for this PR. A small adjustment must be committed before merging!

  3. Jobs for various browsers are run in parallel rather than sequentially. This reduces the amount of time spent on testing and allows for a better overview of running / failing jobs.

  4. BrowserStack connectivity issues are mitigated with BS-specific settings for Karma. karma-browserstack-launcher is set to v1.4 which seems to be much more stable than its newer versions as per this issue.

After these changes there are following scripts in package.json that are related to tests:

  • test:ssr - Tests in SSR mode.
  • test:dry - A new script. It runs react-tester in "dry" mode. react-tests temp folder is NOT created and currently intalled version of React is used. It runs on Chrome by default.
  • test:browser - Convenience wrapper for karma start.
  • test:all - Runs react-tester for all versions of React and on a single browser (Chrome by default).
  • test - Convenience script for running test:dry and linter.

On a sidenote, I tried to get rid of BS altogether, but I was stuck at getting macos / windows images to work with our setup. Installed binaries of browsers (Safari and Edge respectively) were not recognized by Karma. I didn't want to spend more time investigating it.

Closes #191.
Closes #192.

@f1ames f1ames self-assigned this Apr 14, 2021
@f1ames f1ames self-requested a review April 14, 2021 10:58
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Looking good 👍

I was wondering if two separate workflow files makes sense, but I see it keeps them much simpler than putting everything into a single file 👍

Also good job on adding docs to React tester too 👍

And yes, we were considering migrating from Travis to GH Actions for some time so another 👍


Could we also cover IE11 here? We state in the README it's supported (and it is AFAIR) but somehow we have missed adding it to the CI 🙈 😅


Do you think it would make any sense to run additional browsers as a PR check? Or we should trust 3rd-parties (so React and CKEditor 4) simply and assume if it works on Chrome there is 99,9% chance it will work in different browsers too?

Especially, I'm thinking about IE11 which kind of lags behind modern browsers. WDYT?


I know it's a detail but maybe we could simplify job names, now we have:

Maybe we could get rid of the first part ("Check...") and then run time will be also visible? Like:

Alternatively - Run full test suite (Chrome, master), Run tests (Chrome).

I know it's a combination of workflow and job name so maybe be hard to come up with something reasonable 🤔 WDYT?

.travis.yml Show resolved Hide resolved
karma.conf.js Show resolved Hide resolved
package.json Show resolved Hide resolved
.github/workflows/master-check.yml Outdated Show resolved Hide resolved
scripts/react-tester.js Outdated Show resolved Hide resolved
scripts/react-tester.js Outdated Show resolved Hide resolved
scripts/react-tester.js Outdated Show resolved Hide resolved
scripts/react-tester.js Outdated Show resolved Hide resolved
@MMMalik
Copy link
Contributor Author

MMMalik commented Apr 15, 2021

Thanks @f1ames for the review.

I will look into possibility of testing IE11 through BrowserStack.

Running a full suite of tests for each PR is IMO an overkill and I would test all browsers only for pushes to master, stable, and version tags. Let's see how that goes. If we notice that browser-specific bugs are leaking into those branches too often then we can use a stricter approach.

@MMMalik
Copy link
Contributor Author

MMMalik commented Apr 15, 2021

Okay, so I've added some changes:

  • Fixed jsdoc formatting
  • Removed dry completely. For local development use npm run test or npm run test:browser (it will bypass react-tester)
  • Adjusted workflow names. Now they are more compact
  • Support for IE11. This was not that trivial: changes to webpack config in karma.config.js were required (add polyfill, transpile all dependencies). Also, one test was adjusted because it was failing on IE11 (afterEach hook was failing due to some sort of race condition)

@f1ames Please check!

@f1ames f1ames self-requested a review April 16, 2021 10:46
@f1ames f1ames self-assigned this Apr 16, 2021
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Looking good, only some minor polishing here and there and we are good to go 👍

Running a full suite of tests for each PR is IMO an overkill and I would test all browsers only for pushes to master, stable, and version tags. Let's see how that goes. If we notice that browser-specific bugs are leaking into those branches too often then we can use a stricter approach.

Agree 👍

  • Support for IE11. This was not that trivial: changes to webpack config in karma.config.js were required (add polyfill, transpile all dependencies). Also, one test was adjusted because it was failing on IE11 (afterEach hook was failing due to some sort of race condition)

Good job 👍

  • Removed dry completely. For local development use npm run test or npm run test:browser (it will bypass react-tester)

Could you adjust README slightly to mention this (and maybe very short info what other testing commands does)?

scripts/react-tester.js Outdated Show resolved Hide resolved
.github/workflows/test-all.yml Outdated Show resolved Hide resolved
@MMMalik
Copy link
Contributor Author

MMMalik commented Apr 19, 2021

To summarize latest changes:

@MMMalik MMMalik requested a review from f1ames April 19, 2021 08:11
@f1ames f1ames self-assigned this Apr 19, 2021
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Nothing to add here, look good 👍 Good job 👍 🎉

MMMalik and others added 11 commits April 19, 2021 10:55
Refactors various test utils so that tests are run for a requested browser (or ssr) rather than for the whole permuatation of browsers and React versions.
Travis CI is replaced with GitHub workflows. Jobs for various browsers are run in parallel rather than sequentially.
BrowserStack connectivity issues are mitigated with BS-specific settings for Karma. Karma launcher for BS is set to v1.4 (more stable than newer versions).
Co-authored-by: Krzysztof Krztoń <[email protected]>
@f1ames
Copy link
Contributor

f1ames commented Apr 19, 2021

Rebased onto latest master.

@f1ames f1ames merged commit 2e1762d into master Apr 19, 2021
@f1ames f1ames deleted the t/191 branch April 19, 2021 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit number of browsers / React versions when testing PRs BrowserStack connectivity issues
2 participants