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

Fix CI tests #294

Merged
merged 12 commits into from
Jan 11, 2023
Merged

Fix CI tests #294

merged 12 commits into from
Jan 11, 2023

Conversation

jcmosc
Copy link
Collaborator

@jcmosc jcmosc commented Jan 9, 2023

Fixes CI tests, as much as possible from a forked repo.

Note: This PR bumps the minimum Node version from 12 to 17. This is because browser tests use an older version of Webpack that requires NODE_OPTIONS=--openssl-legacy-provider to be specified when using recent versions of Node. However this option does not exist in older version of Node.

Blank binary file was not getting serialized as { type: "Buffer", data: [] }
because read(file) was returning a string instead of Buffer.
Recent versions of node upgraded to OpenSSL 3.0, which deprecated some
older crypto hashing algorithms including md4.

Webpack 4 hard codes the use of md4 ins some places, so we have to set this
flag until we can upgrade Webpack.
Browser tests were failing as polyfill.js was loading a node package in a browser context.
This was when the --openssl-legacy-provider option was introduced.
Prior versions of node do not recognize this option.
Browser tests are failing in the CI environment with:
chokidar@3: Error: Cannot find module 'chokidar'

Don't know why but this thread might be relevant:
facebook/create-react-app#10811
This was referenced Jan 9, 2023
@philsturgeon
Copy link
Member

thanks for this! nearly there, do you know whats up with ubuntu browser tests?

@jcmosc
Copy link
Collaborator Author

jcmosc commented Jan 9, 2023

It's failing because it can't launch Edge.

09 01 2023 11:25:29.297:INFO [launcher]: Starting browser Edge
09 01 2023 11:25:29.298:ERROR [launcher]: No binary for Edge browser on your platform.
  Please, set "EDGE_BIN" env variable.

To be honest I was hoping there would be something different about the Sauce Labs configuration in this repo than my fork that would make this work. Will need to investigate further.

But maybe we don't need it for Edge anyway as it's included in the runner image?
https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md

@philsturgeon
Copy link
Member

Anything we can do to get away from SauceLabs is a good thing IMO. It's been a nightmare trying to keep it happy.

@jcmosc
Copy link
Collaborator Author

jcmosc commented Jan 11, 2023

Ok that's good to know.

These commits should now pass. https://github.com/criteria-labs/json-schema-ref-parser/actions/runs/3888908655

I got rid of Sauce Labs and browsers are now just tested on their "home" platform, i.e. Edge on windows, Safari on macOS. Though there is no runner for macos-latest because the karma launcher for Safari doesn't work (it hasn't been updated in 9 years). So there are no Safari tests at all.

Copy link
Member

@philsturgeon philsturgeon left a comment

Choose a reason for hiding this comment

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

Looming good. No bother about safari, it was already disabled.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3889025475

  • 6 of 8 (75.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.632%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/resolvers/http.js 4 6 66.67%
Totals Coverage Status
Change from base Build 3554282655: 0%
Covered Lines: 841
Relevant Lines: 876

💛 - Coveralls

@philsturgeon philsturgeon merged commit da9e32e into APIDevTools:main Jan 11, 2023
@zomars
Copy link

zomars commented Jan 20, 2023

This is breaking most apps that rely on Node 16:

image

@Elhebert
Copy link

We also had our app breaking without warning due to this.

I feel like this should have been released as a new major version. Changing the minimum Node version is a breaking change after all.

@@ -43,7 +43,7 @@
"fs": false
},
"engines": {
"node": ">= 14"
"node": ">= 17"
Copy link

@AviVahl AviVahl Jan 23, 2023

Choose a reason for hiding this comment

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

@jamesmoschou this caused a regression on our end as well. just having it in the tree of a node v16.x project breaks during dependency installation (yarn, in our case), even if we don't directly use this package.

EDIT: nvmind found #298

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.

None yet

6 participants