-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
There was a problem hiding this 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?
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 |
Okay, so I've added some changes:
@f1ames Please check! |
There was a problem hiding this 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 usenpm run test
ornpm run test:browser
(it will bypassreact-tester
)
Could you adjust README slightly to mention this (and maybe very short info what other testing commands does)?
To summarize latest changes:
|
There was a problem hiding this 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 👍 🎉
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]>
Rebased onto latest |
This PR achieves the following:
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.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 tomaster
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!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.
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 runsreact-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 forkarma start
.test:all
- Runsreact-tester
for all versions of React and on a single browser (Chrome by default).test
- Convenience script for runningtest: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.