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

Updating Cypress from version 12.1.0 to 12.3.0 #2669

Merged
merged 8 commits into from
Feb 6, 2023
Merged

Updating Cypress from version 12.1.0 to 12.3.0 #2669

merged 8 commits into from
Feb 6, 2023

Conversation

RobertoCassino
Copy link
Contributor

What change does this PR introduce?

This PR updates the cypress at apps/web/package.json (line 129) from version 12.1.0 to version 12.3.0.

Why was this change needed?

4 CVEs were found in novu, the version of the Cypress testing framework is the root cause here, as the version used is 12.1.0 and it includes and old version of the packages like simple-git and others.

@ghost
Copy link

ghost commented Feb 2, 2023

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@scopsy scopsy self-requested a review February 2, 2023 11:01
@RobertoCassino RobertoCassino marked this pull request as ready for review February 2, 2023 11:38
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

Mind you to update it also in /apps/widget so we have the versions used in both apps aligned?

@RobertoCassino RobertoCassino requested review from p-fernandez and removed request for scopsy February 2, 2023 15:31
@scopsy
Copy link
Contributor

scopsy commented Feb 2, 2023

Let's see of tests will pass, last time i tried updating widget it didn't worked out :( Two major versions upgrade, been a few breaking changes there since

@@ -69,7 +69,7 @@
"@types/react-router-dom": "^5.1.7",
"craco-antd": "^1.19.0",
"cross-env": "^7.0.3",
"cypress": "10.11.0",
"cypress": "12.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"cypress": "12.3.0",
"cypress": "^12.3.0",

So we both get it updated (web and widget) and just one single version in the dependency all the time.

Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

🌟

@RobertoCassino
Copy link
Contributor Author

🌟

Thanks @p-fernandez for approval, the end to end test for widget is still failing and I am not sure what the problem is for the api migration. We focused on replacing .should() with .then() in accordance to the manual provided by Cypress https://docs.cypress.io/guides/references/migration-guide#should. The test is now failing at getNotifications() on line 161 in apps/widget/cypress/e2e/shell-embed.spec.ts. This is weird, as the function is being used in multiple tests which are passing successfully.

The exact point of failure is on line 167 where is trying to execute cy.wrap(body).find([data-test-id="notification-list-item"]);. What would the next steps be?

@p-fernandez
Copy link
Contributor

🌟

Thanks @p-fernandez for approval, the end to end test for widget is still failing and I am not sure what the problem is for the api migration. We focused on replacing .should() with .then() in accordance to the manual provided by Cypress https://docs.cypress.io/guides/references/migration-guide#should. The test is now failing at getNotifications() on line 161 in apps/widget/cypress/e2e/shell-embed.spec.ts. This is weird, as the function is being used in multiple tests which are passing successfully.

The exact point of failure is on line 167 where is trying to execute cy.wrap(body).find([data-test-id="notification-list-item"]);. What would the next steps be?

We might need to revisit the test. Just double checking it is not a timeout issue from Cypress.

@LetItRock
Copy link
Contributor

@RobertoCassino I've fixed failing widget tests, hope they will pass now ;)

@LetItRock
Copy link
Contributor

@RobertoCassino @p-fernandez @scopsy
I see that the test is failing because of the race conditions in the cache, I'll inform @djabarovgeorge

@scopsy
Copy link
Contributor

scopsy commented Feb 5, 2023

@LetItRock seems like it's a flaky tests that also happens regardless of this PR. My suggestion is to merge this one and deal with the race condition in another PR. WDYT?

@scopsy scopsy merged commit 952a2c1 into novuhq:next Feb 6, 2023
@pieterge
Copy link

pieterge commented Feb 6, 2023

Crazy question: When would the next release be? Do you guys have a default cycle?

@scopsy
Copy link
Contributor

scopsy commented Feb 6, 2023

@pieterge end of the month are the usual monthly cadence, however I plan to release a hotfix release until the end of the week 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants