-
Notifications
You must be signed in to change notification settings - Fork 254
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
Update Cypress and Jest versions and remove Node 8 support #543
Conversation
It looks like it was useful to remove the container after the run.
It seems like a bug in Cypress when you use the `after` function.
🦋 Changeset is good to goLatest commit: a178a8c We got this. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ESLint Summary View Full Report
[warning] jest/no-disabled-tests
Report generated by eslint-plus-action |
Skip because it started failing when we upgraded to Jest 26, but we are going to remove it once we merge this PR anyway: #415.
Codecov Report
|
/* eslint-disable */ | ||
/* | ||
* TSDocs will be added in this branch: | ||
* https://github.com/frontity/frontity/tree/package-name |
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.
Is this correct? I see that the last commit on that branch was 8 days ago.
Oh, I see that it's in other files as well. I assume that you had a good reason for doing this but in general I would say that we shouldn't keep track of tasks this way because it's asking for trouble once it's merged and someone forgets 😅
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.
You're right, it's not a good approach. I should've copied the TSDocs from that branch 👍
@@ -25,7 +25,7 @@ module.exports = (on, config) => { | |||
installPlugin({ name, version }) { | |||
return (async () => { | |||
await execa.command( | |||
`docker-compose run wpcli wp plugin install ${name}${ | |||
`docker-compose run --rm wpcli wp plugin install ${name}${ |
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.
👍 when running locally it leaves lotsa containers behind, good point.
@@ -9,7 +9,7 @@ const mockedHe = he as jest.Mocked<typeof he>; | |||
describe("decode", () => { | |||
beforeEach(() => { | |||
mockedHe.decode.mockReset(); | |||
mockedHe.decode.mockImplementation(require.requireActual("he").decode); | |||
mockedHe.decode.mockImplementation(jest.requireActual("he").decode); |
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.
Just leaving this for reference: jestjs/jest#9854. This change is because of jest deprecating require.requireActual()
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.
Not strictly related to the changes in the PR, but I've noticed that the URL
constructor is not supported at all in IE so as far as I can tell that means that we don't support IE at all.
Based on https://community.frontity.org/t/frontity-support-for-ie9/2540/5?u=mmczaplinski we decided to support IE11 SSR-only so that would break this promise.
The client-code it's not executed in IE because it's server-side only. So This hasn't changed in this PR. We were already using Thanks for the review and the comments @michalczaplinski! 🙂 |
ESLint Summary View Full Report
[warning] jest/no-disabled-tests
Report generated by eslint-plus-action |
oh, I think I was tired yesterday since the answer was in my own question basically 🤦♂️ Thanks @luisherranz :) |
Haha, that's true 😄 |
What:
I've updated Cypress and Jest to their latest versions.
I've deprecated the
URL
import from"frontity"
, which was the only thing we had for backward compatibility with Node 8.I've also fixed a couple of issues with Cypress e2e tests:
run
.Why:
We've made several changes to our test system, I think it's good to keep it up to date with the latest Jest/Cypress versions.
Nobody supports Node 8 anymore. I think it's not even on maintenance anymore. All serverless platforms have updated to either Node 10 or Node 12. https://nodejs.org/en/about/releases/
How:
Simply by updating the dependencies and creating a wrapper around
URL
to warn about the deprecation.Tasks:
Unrelated Tasks
Additional Comments
I've added a dummy test to the end of the
wp-basic-tests
e2e tests, because if not Cypress was ignoring the previous test. I guess this is a bug in Cypress, maybe related to theafter
function.