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

feat(ci,deps): Add CI workflow to enforce general Internet Explorer compatibility #153

Merged
merged 13 commits into from
Oct 28, 2021

Conversation

tehhowch
Copy link
Contributor

@tehhowch tehhowch commented Oct 27, 2021

Enforces that Internet Explorer will at least be able to parse interceptor.js. IE was already not able to execute all code paths unless the website in question has polyfilled both fetch and Promise.all.

This PR addresses #152 partially, in that it provides a basic guarantee that the library is not utterly broken for Internet Explorer. This PR does not aim to fulfill all of #152, specifically actually executing tests for all supported browsers.

Other changes

  • upgrade husky, prettier to latest versions
  • fix prettier invocation by npm script (would exit with status 2 previously)
  • switch CI vm hosts from macos to linux, since GitHub's macOS hosts are much less available than the Ubuntu ones.

 - modern JS most places
 - ES5 in interceptor, to ensure Internet Explorer's JS runtime can parse the script without breaking
   - Not all code paths are _executable_ by the IE runtime, though!
not correctly supported by IE < 11, and poorly supported in IE 11
 - commit results of `npm run fix`
 - use linux hosts, rather than macos, since they are much more available
 - use setup-node v2, with npm caching
run it on every push, because contributors shouldn't have to wait until they open a PR _and_ a maintainer approves a CI run to get feedback
@tehhowch
Copy link
Contributor Author

@tehhowch
Copy link
Contributor Author

@chmanie you can approve the CI again; the chromedriver dependency was out of date. I had updated it locally so tests could run.

The newly added workflows will not run here; they need to be on the default branch for GitHub to pick them up. You can see the ones that execute on my fork here: https://github.com/tehhowch/wdio-intercept-service/actions

@chmanie
Copy link
Member

chmanie commented Oct 27, 2021

This looks brilliant! Thanks for doing the spring cleaning here. Once the tests pass, I think we can merge this.

@tehhowch if you're planning on putting up more PRs here, I'll happily give you write access, so that the flow here will be a bit smoother.

Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

I would only suggest to jump to Node v16. Maybe we can also add an .nvmrc file?

@tehhowch
Copy link
Contributor Author

I would only suggest to jump to Node v16. Maybe we can also add an .nvmrc file?

This repo uses sync wdio for tests, so that cannot be done yet. I don't plan on converting to async in this PR either.

@christian-bromann
Copy link
Contributor

This repo uses sync wdio for tests, so that cannot be done yet. I don't plan on converting to async in this PR either.

Makes sense 👍

@tehhowch
Copy link
Contributor Author

@christian-bromann thoughts on why the CI job fails to execute the test? (I could add an artifact upload step to share the logs directory on failure, but that's probably not wise for a public resource which could eventually include secrets, if someone implements third-party testing VMs.)

@christian-bromann
Copy link
Contributor

@tehhowch yeah, would be great to see the wdio logs. The output dir is defined as outputDir: __dirname + '/logs', so we just need to capture all the logs in that directory. Does it work locally for you?

@tehhowch tehhowch force-pushed the ci/add-eslint branch 2 times, most recently from 36a27c6 to 3b43f43 Compare October 28, 2021 13:01
@tehhowch
Copy link
Contributor Author

@christian-bromann relevant log message is from the test log (wdio-0-0.log):

2021-10-28T12:41:15.820Z WARN webdriver: Request failed with status 500 due to session not created: This version of ChromeDriver only supports Chrome version 95
Current browser version is 94.0.4606.81 with binary path /usr/bin/google-chrome

Seems like this particular issue should be an error, not a warning, and get surfaced in the actual test output rather than just logs. (Going off of the "status 500" bit as a proxy for "internal server error").

Anyway, resolved by updating the wdio-chromedriver-service to 7.2 and passing the chromedriverCustomPath option, using the chromedriver that is shipped in the VM by GitHub.

@tehhowch
Copy link
Contributor Author

tehhowch commented Oct 28, 2021

Looks like the CI tests are incredibly slow due to a forced 10s wait per test case. Seems like the functionality of #111 would really help accelerate the tests and remove the "how long do we need to wait?" uncertainty.

Link to test run: https://github.com/tehhowch/wdio-intercept-service/actions/runs/1394780199

@christian-bromann
Copy link
Contributor

Anyway, resolved by updating the wdio-chromedriver-service to 7.2 and passing the chromedriverCustomPath option, using the chromedriver that is shipped in the VM by GitHub.

Nice!

@tehhowch
Copy link
Contributor Author

@chmanie I think we're set here; I did some bonus cleanup on the old Renovate integration. There may be some administrative settings that need to be ticked in addition to the dependabot config added here to restore automatic dependency upgrades, but I think that should be all.

@chmanie
Copy link
Member

chmanie commented Oct 28, 2021

This looks great! Thanks a lot! Really appreciate it

Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Absolutely agree, great job! Thanks 👍

@christian-bromann christian-bromann merged commit 9f7db58 into webdriverio-community:main Oct 28, 2021
@tehhowch tehhowch deleted the ci/add-eslint branch October 28, 2021 15:54
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.

3 participants