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

upgraded cypress 11.x -> 13.x #6912

Closed

Conversation

AndrewChubatiuk
Copy link
Collaborator

@AndrewChubatiuk AndrewChubatiuk commented Apr 20, 2024

What type of PR is this?

  • upgraded cypress to fix this issue

  • updated db seed function which helped to remove not needed request-cookie package

  • removed unneeded cypress dockerfile to speed up CI build (temporary added image to pass CI checks)

  • updated dockerignore with more unused content for redash image

  • removed compose profiles

  • Refactor

  • Feature

  • Bug Fix

  • New Query Runner (Data Source)

  • New Alert Destination

  • Other

Description

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@AndrewChubatiuk AndrewChubatiuk changed the title Updated docker build upgraded cypress and axios Apr 20, 2024
@AndrewChubatiuk AndrewChubatiuk changed the title upgraded cypress and axios upgraded cypress 11.x -> 13.x Apr 21, 2024
@AndrewChubatiuk AndrewChubatiuk force-pushed the updated-docker-build branch 9 times, most recently from 4e68ddc to 892699f Compare April 22, 2024 11:59
@AndrewChubatiuk
Copy link
Collaborator Author

@justinclift could you please review when have time?

@gaecoli
Copy link
Member

gaecoli commented Apr 24, 2024

I don't understand this part very well. Come and take a look. @justinclift 😁

@justinclift
Copy link
Member

@gaecoli All good, I'll take a look at this in a few hours. 😄

@justinclift justinclift self-assigned this Apr 24, 2024
@AndrewChubatiuk
Copy link
Collaborator Author

@gaecoli All good, I'll take a look at this in a few hours. 😄

@justinclift please merge this PR before other ones in a repo to speed up e2e tests execution for other pipelines

@justinclift
Copy link
Member

please merge this PR before other ones in a repo to speed up e2e tests execution for other pipelines

Sorry, just saw that. I don't have time to look at this properly for a few more hours though, so it'll have to wait until then.

@gaecoli gaecoli removed their request for review April 24, 2024 14:03
@justinclift
Copy link
Member

Sorry @AndrewChubatiuk, this will have to be a tomorrow thing instead of today. I've run out of time and mental energy for today. 🤦

Unless someone else gets to it overnight of course. 😄

@AndrewChubatiuk
Copy link
Collaborator Author

@justinclift do you have time for it today?

@justinclift
Copy link
Member

Yeah. I just finished most of the other bits I need to take care of today, so I'll grab some food now then go through this. 😄

@@ -14,31 +12,49 @@ try {

Copy link
Member

Choose a reason for hiding this comment

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

@eradman Would you have time to look over this client/cypress/cypress.js file?

The changes in it are more complex than I'm able to really grok atm.

The changes in the Cypress tests below though seem workable. And it'd be good to get the Cypress pieces updated past Cypress 12.x. 😄

@justinclift
Copy link
Member

@AndrewChubatiuk This mostly looks good. I've asked @eradman to take a look at the most complex JS file (cypress.js) in there, as it's more complex than what I can currently grok.

removed compose profiles

Will that affect people who run the Cypress tests manually themselves? ie from the cli, prior to making a PR

@AndrewChubatiuk
Copy link
Collaborator Author

@justinclift updated makefile, so it should not affect existing user's setup

Copy link
Collaborator

@eradman eradman left a comment

Choose a reason for hiding this comment

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

From my perspective this change is too large. We need to break this update into smaller steps so that we are able to fully understand.

@AndrewChubatiuk
Copy link
Collaborator Author

From my perspective this change is too large. We need to break this update into smaller steps so that we are able to fully understand.

i can help if you have any questions. most of the changes are happening in yarn.lock, cause lot's of old packages were removed after percy upgrade

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.76%. Comparing base (af0773c) to head (9d8e7d2).
Report is 35 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6912      +/-   ##
==========================================
- Coverage   63.82%   63.76%   -0.06%     
==========================================
  Files         161      161              
  Lines       13060    13066       +6     
  Branches     1803     1803              
==========================================
- Hits         8335     8332       -3     
- Misses       4425     4431       +6     
- Partials      300      303       +3     

see 1 file with indirect coverage changes

@AndrewChubatiuk
Copy link
Collaborator Author

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.

puppeteer issue with Apple Silicon chips
4 participants