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

#826 Update Cypress #1042

Merged
merged 5 commits into from
Jan 3, 2021
Merged

#826 Update Cypress #1042

merged 5 commits into from
Jan 3, 2021

Conversation

snowteamer
Copy link
Collaborator

@snowteamer snowteamer commented Dec 26, 2020

Summary of changes

  • Updated Cypress to 6.2.0
  • Had to change one line in group-contributions.spec.js to use not.exist instead of not.be.visible (see nonexistent element assertions in the Cypress migration guide).
  • Temporarily downgrade Node from v15 to v14 to avoid a dependency compatibility issue that was found by running the tests. Some of our dependencies do not support Node v15 yet.

Additional info

The overall Cypress run for this build took 3min 40s, which is 23s better than the run from #1041, and 14s better than the run from #1040.

Therefore, the performance issues mentionned in #829 seem to have been resolved, or to no longer be relevant.

As for the other issues, I'm not sure. There are a lot of open issues on the Cypress repo, but I didn't go as far as investigating enough of them to guess if the situation was better in version 3.5.

Maybe @sandrina-p could tell whether we can safely upgrade?

Closes #1042

@snowteamer snowteamer force-pushed the update-cypress branch 2 times, most recently from 51f062e to 4023713 Compare December 26, 2020 19:22
@snowteamer snowteamer changed the title [WIP] Update Cypress WIP: Update Cypress Dec 26, 2020
@snowteamer snowteamer self-assigned this Dec 27, 2020
@snowteamer snowteamer linked an issue Dec 27, 2020 that may be closed by this pull request
@snowteamer snowteamer changed the title WIP: Update Cypress #826 Update Cypress Dec 27, 2020
Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

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

Awesome PR @snowteamer!

One request: could you please update your version of node and npm? The reason I ask is because using the latest version will produce a different package-lock.json file. We'd like to use the latest version of it so as to avoid conflicts in PRs going forward.

E.g., when I test locally, I get a large diff after running npm install that has a different lockfileVersion:

-> % git diff   

diff --git a/package-lock.json b/package-lock.json
index 2bdb51d7..48d74d2c 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,8 +1,20071 @@
 {
   "name": "group-income-simple",
   "version": "0.0.1",
-  "lockfileVersion": 1,
+  "lockfileVersion": 2,

When I run npm version, here are the values I get:

-> % npm version
{
  'group-income-simple': '0.0.1',
  npm: '7.0.15',
  node: '15.4.0',

 [ ... snip irrelevant values ... ]

You can update node to 15 using whatever package manager you're using, and if it doesn't update npm to at least 7.0.15 after doing that, then you can do it yourself by running npm i -g npm@latest.

Finally, feel free to bump the minimum node version requirements in the README.md from 12 to 15.

@snowteamer
Copy link
Collaborator Author

That's indeed a good idea, now that our Travis config no longer mandates using Node v14.

However, since this might be useful for other contributors, I will add that npm i -g npm@latest will not upgrade npm from v6.x to v7.x, but only to the latest v6.x version.

So I had to run npm i -g npm@7, explicitly specifying the major version number.

@snowteamer
Copy link
Collaborator Author

Hmm this still causes a merge conflict and a rather large diff - should I temporarily use the exact Node and NPM versions listed in your console output?

@taoeffect
Copy link
Member

@snowteamer Let's try to troubleshoot this on Slack

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

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

Excellent work @snowteamer! Thank you for sending this in! 😄

@taoeffect taoeffect merged commit 2c6edee into okTurtles:master Jan 3, 2021
@snowteamer snowteamer deleted the update-cypress branch January 4, 2021 03:20
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.

Upgrade us to Cypress 4.0.1 or greater
2 participants