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: filtering with --spec option for projects in Docker container root #23535

Merged
merged 9 commits into from
Aug 29, 2022

Conversation

tbiethman
Copy link
Contributor

@tbiethman tbiethman commented Aug 24, 2022

User facing changelog

  • Fixed an issue where filtering with the --spec run mode option would not find any specs if the project was located at the root directory of a file system.

Additional details

This logic has been changed a few times lately to ensure that we are only globbing user-provided patterns. These changes introduced a regression where, with a working directory of root (/), the glob pattern would be incorrectly manipulated:

working dir of / + glob pattern of /foo/bar/*.cy.js => .foo/bar/*.cy.js // this won't match anything

This typically only occurred with our docker images, where users may mount project content directly to the container's root. We correct this issue by having better checks around our absolute glob pattern detection and deriving implicit relative patterns (foo/bar/*.cy.js rather than ./foo/bar/*.cy.js) for the glob patterns that we must manipulate.

Steps to test

The added unit tests now show the root working directory scenarios being properly handled.

I also created a validation repo here: https://github.com/tbiethman/verify-23535. Steps to validate are included on its README.

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [n/a] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a] Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 24, 2022

Thanks for taking the time to open a PR!

@@ -9,6 +9,10 @@ import { toPosix } from '../util/file'

const debug = Debug('cypress:data-context:sources:FileDataSource')

export const matchGlobs = async (globs: string | string[], globbyOptions?: GlobbyOptions) => {
return await globby(globs, globbyOptions)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our pinned version of globby exports the function itself as the module export, making it difficult to stub calls to it for unit testing purposes.

So I added a little wrapper here so I could stub these calls appropriately, but I can remove this if I'm missing an obvious way to stub the module directly.


expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.been.calledWith(
['cypress/e2e/**.cy.js'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 10.6.0, this test case would produce .cypress/e2e/**.cy.js, which is not a valid pattern.

@cypress
Copy link

cypress bot commented Aug 25, 2022



Test summary

39641 0 3371 0Flakiness 0


Run details

Project cypress
Status Passed
Commit e15581b
Started Aug 29, 2022 5:35 PM
Ended Aug 29, 2022 5:49 PM
Duration 14:14 💡
OS Linux Debian - 11.3
Browser Multiple

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

@flotwig flotwig self-requested a review August 26, 2022 15:34
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the unit tests here!

Comment on lines +45 to 49
if (globPattern.startsWith(workingDirectoryPrefix)) {
return globPattern.replace(workingDirectoryPrefix, '')
}

return globPattern
Copy link
Contributor

@flotwig flotwig Aug 29, 2022

Choose a reason for hiding this comment

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

Would return path.relative(workingDirectoryPrefix, globPattern) also work instead of these 5 lines? Might not even need line 39 at that point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think path.relative has the potential to mutate the glob patterns to be relative to the current execution directory, which may or may not equal the given cwd argument. In those cases, the pattern would no longer be representative of what the user expects.

@@ -9,6 +9,10 @@ import { toPosix } from '../util/file'

const debug = Debug('cypress:data-context:sources:FileDataSource')

export const matchGlobs = async (globs: string | string[], globbyOptions?: GlobbyOptions) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

An async function with return await is equivalent to a non-async function that just returns the promise.

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.

Using --spec option results in "no specs found" when project is located at Docker root directory
3 participants