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: e2e percy snapshots #20604

Closed
wants to merge 30 commits into from
Closed

Conversation

brian-mann
Copy link
Member

@brian-mann brian-mann commented Mar 14, 2022

Details

Allows static vue assets to be accessible without going through the proxy.

Percy generates local snapshots outside of Cypress in their own browser without configuring the proxy server. Our internal proxy server normally rejects requests made outside of the browser (in the proxy mode). This change allows static assets for vite to be served.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 14, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Mar 14, 2022



Test summary

4333 0 48 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 400926d
Started Mar 24, 2022 3:26 PM
Ended Mar 24, 2022 3:38 PM
Duration 12:00 💡
OS Linux Debian - 10.10
Browser Electron 94

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@tbiethman tbiethman changed the title Fix: e2e percy snapshots fix: e2e percy snapshots Mar 15, 2022
@brian-mann brian-mann marked this pull request as draft March 16, 2022 13:31
lmiller1990
lmiller1990 previously approved these changes Mar 22, 2022
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Seems fine - good solution. For anyone else looking at this PR - there's a ton of renames here, the main change is in this file: https://github.com/cypress-io/cypress/pull/20604/files#diff-040b9d41ce1efd9534f52c69af25d3d587c3a5b9cc9cb8b39a2848ec3e7e420e.

I also pulled the branch locally and tested a bit, everything seems 💯 . We had better approve the remaining 18 snapshots on Percy before merging (since they are now showing correctly).

@@ -48,15 +48,39 @@ const _forceProxyMiddleware = function (clientRoute, namespace = '__cypress') {
`/${namespace}/runner/cypress_runner.css`,
`/${namespace}/runner/cypress_runner.js`, // TODO: fix this
`/${namespace}/runner/favicon.ico`,

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, good solution

@lmiller1990 lmiller1990 marked this pull request as ready for review March 22, 2022 04:26
ryanthemanuel
ryanthemanuel previously approved these changes Mar 22, 2022
@@ -1,10 +1,9 @@
import { makeConfig } from '../frontend-shared/vite.config'
import Layouts from 'vite-plugin-vue-layouts'
import Pages from 'vite-plugin-pages'
import Copy from 'rollup-plugin-copy'
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for that !!
Great clean-up.

Should we also remove the plugin from the dependencies in package.json?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, I went ahead and removed it.

@tbiethman
Copy link
Contributor

@brian-mann Is this PR ready for review? I know you are working on cleaning up some of the snapshots, but I'm not sure if you are wanting to merge those changes here or in a follow-up PR.

@lmiller1990 lmiller1990 dismissed stale reviews from ryanthemanuel and themself via 061d9ed March 24, 2022 04:04
@lmiller1990 lmiller1990 requested a review from a team as a code owner March 24, 2022 04:04
@lmiller1990
Copy link
Contributor

lmiller1990 commented Mar 24, 2022

I went ahead and merged in 10.0-release, resolved conflicts, and made the trivial review changes (typos, remove unused dep from package.json.

@tbiethman it's marked as ready and in review - I'd assume we are good to review/merge. It seems like a good place to merge - CI is green.

@lmiller1990 lmiller1990 self-requested a review March 24, 2022 04:11
@jennifer-shehane
Copy link
Member

I confirmed with @brian-mann - he said this should still be in draft, he has some things to finalize. @lmiller1990 if you'd like you can sync with Brian to see what he wanted done and see if you can take over the tasks. Otherwise it's not ready for review.

@jennifer-shehane jennifer-shehane marked this pull request as draft March 24, 2022 15:38
@jennifer-shehane jennifer-shehane self-requested a review April 11, 2022 14:45
@jennifer-shehane jennifer-shehane removed request for a team and tbiethman April 21, 2022 14:47
@jennifer-shehane jennifer-shehane removed their request for review April 28, 2022 15:13
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

@brian-mann Update the base branch to be against develop before merging.

@emilyrohrbough
Copy link
Member

Closing as stale. We can re-visit at a later time.

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.

7 participants