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: resolve files outside the project directory to the bundled directory #30067

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Aug 20, 2024

Additional details

Files that live outside of the user's project directory that are referenced by the project (ex. ../support.js support file reference inside cypress.config.js) did not resolve correctly on Windows. This is only noticable on Windows since the absolute path gets appended to the project bundle. In Unix based systems, this goes unnoticed because /Users/foo/project/nested/hash-bundle/Users/foo/project/file.js is a valid path in Unix, but C:\Users\foo\project\nested\hash-bundleC:\Users\foo\project\file.js is not a valid path in Windows

To resolve this issue, we find the common ancestor directory between the project and file, take the path AFTER the common ancestor directory of the file, and append it to the project bundle directory. Effectively C:\Users\foo\project\nested\hash-bundleC:\Users\foo\project\file.js becomes C:\Users\foo\project\nested\hash-bundle\file.js. Collisions here are technically possible, but it seems highly unlikely.

I was originally going to use a library called common-ancestor-path, but it was fairly difficult to mock the path dependencies to get the correct separator when trying to mock win32 paths inside a posix context. The library was small enough to modify for our needs that I added in the app_data file.

Before

After

Steps to test

Tests for the findCommonAncestor and the getBundledFilePath fixes are unit tested. I contemplated adding a system-test, but because the support file would have to live outside the project directory (or some other spec) and would need to run it on windows specifically, I opted to not add one since the maintenance cost seems to outweigh the benefits. Reevaluating, I think the unit tests here should give us enough coverage for what we expect, but I do not feel great about how much stubbing has to happen to test win32 libraries on posix.

If you want to test this fix fully end to end, you can clone this fork on windows and test with the 13.13.3 binary to see the failure, then install the binary built on this branch, https://cdn.cypress.io/beta/npm/13.14.0/win32-x64/fix/8599-e6b74fd2fa67f8731a80cda7d12ce23d0d5d9fd0/cypress.tgz, and see the issue is fixed. The file will now be available outputted by webpack in the bundle hash directory.

Note: You will not be able to reproduce the issue meaningfully on mac or linux.

How has the user experience changed?

PR Tasks

…tory via the directory path difference [run ci]
@AtofStryker AtofStryker self-assigned this Aug 20, 2024
@AtofStryker AtofStryker changed the title fix: resolve files outside the project directory to the bundled direc… fix: resolve files outside the project directory to the bundled directory Aug 20, 2024
Copy link

cypress bot commented Aug 20, 2024

cypress    Run #56721

Run Properties:  status check passed Passed #56721  •  git commit 08f539fa27: update common ancestor code
Project cypress
Branch Review fix/8599
Run status status check passed Passed #56721
Run duration 52m 12s
Commit git commit 08f539fa27: update common ancestor code
Committer AtofStryker
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 26
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 769
View all changes introduced in this branch ↗︎
UI Coverage  65.48%
  Untested elements 27  
  Tested elements 55  
Accessibility  96.6%
  Failed rules  0 critical   8 serious   3 moderate   0 minor
  Failed elements 210  

@AtofStryker AtofStryker marked this pull request as ready for review August 20, 2024 18:09
@SidMarti
Copy link

Binary file provided seems to fix the issue on our end. Thanks

@AtofStryker AtofStryker merged commit d79c99e into develop Aug 22, 2024
103 of 107 checks passed
@AtofStryker AtofStryker deleted the fix/8599 branch August 22, 2024 20:59
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 27, 2024

Released in 13.14.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.14.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: EINVAL: invalid argument, mkdir on windows for cypress + windows + Angular project
5 participants