-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Encode fileServerFolder to fix #25839 #29162
fix: Encode fileServerFolder to fix #25839 #29162
Conversation
|
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.
Thanks for the contribution! Could you add a test around this behavior? We'll need a test in order to merge the fix.
@jennifer-shehane |
@jennifer-shehane Thanks for your review! |
Dismissing previous review around added test
@jennifer-shehane I'm a bit confused right now, sorry. Is there anything else need me to do, or just wait for you to verify and initiate the merge? |
@Zzet-Z Just waiting til next sprint to see if I can assign some more eyes to review this. |
OK thx! |
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.
@Zzet-Z There appear to be some failing tests with this change: https://app.circleci.com/pipelines/github/cypress-io/cypress/61086/workflows/6745104d-7a56-4c94-a750-d7c5a415b06e/jobs/2539954
@jennifer-shehane Oh, that's a problem, and I found some other problems and dealt with them. The local operation passed these tests. Could you please help me run the integration test again if it is convenient. |
@Zzet-Z Running them now |
@jennifer-shehane Oops,I neglected a test case, now it is updated, please help run it again when you are free.By the way, there are still a small part of failures that seem not to be caused by my changes, could you please help me check whether these need my attention, thx! |
Dismissing my previous review to open up reviews for the team
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
When there are invalid header characters in a test folder path (such as Chinese characters), setting the
x-cypress-file-path
header fails. This PR will encode the file path header value to solve the issue and decode it inserver-base.ts
.Steps to test
cy.visit
s a file served from the local file serverHow has the user experience changed?
The
Invalid character in header content ["x- cypress-file-path"]
error is no longer thrown.PR Tasks
cypress-documentation
?type definitions
?