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: Suppress filesystem errors during glob search #25774

Merged
merged 4 commits into from
Feb 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ _Released 02/14/2023 (PENDING)_
- Fixed an issue in middleware where error-handling code could itself generate an error and fail to report the original issue. Fixes [#22825](https://github.com/cypress-io/cypress/issues/22825).
- Fixed an issue that could cause the Debug page to display a different number of specs for in-progress runs than shown in Cypress Cloud. Fixes [#25647](https://github.com/cypress-io/cypress/issues/25647).
- Fixed an issue introduced in Cypress 12.3.0 where custom browsers that relied on process environment variables were not found on macOS arm64 architectures. Fixed in [#25753](https://github.com/cypress-io/cypress/pull/25753).
- Fixed an issue where Cypress would fail to load any specs if the project `specPattern` included a resource that could not be accessed due to filesystem permissions. Fixes [#24109](https://github.com/cypress-io/cypress/issues/24109).

**Misc:**

Expand Down
14 changes: 11 additions & 3 deletions packages/data-context/src/sources/FileDataSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class FileDataSource {
return this.ctx.fs.readFile(path.join(this.ctx.currentProject, relative), 'utf-8')
}

async getFilesByGlob (cwd: string, glob: string | string[], globOptions?: GlobbyOptions) {
async getFilesByGlob (cwd: string, glob: string | string[], globOptions: GlobbyOptions = {}): Promise<string[]> {
const globs = ([] as string[]).concat(glob).map((globPattern) => {
const workingDirectoryPrefix = path.join(cwd, path.sep)

Expand All @@ -49,7 +49,7 @@ export class FileDataSource {
return globPattern
})

const ignoreGlob = (globOptions?.ignore ?? []).concat('**/node_modules/**')
const ignoreGlob = (globOptions.ignore ?? []).concat('**/node_modules/**')

if (os.platform() === 'win32') {
// globby can't work with backwards slashes
Expand All @@ -72,7 +72,15 @@ export class FileDataSource {

return files
} catch (e) {
debug('error in getFilesByGlob %o', e)
if (!globOptions.suppressErrors) {
// Log error and retry with filesystem errors suppressed - this allows us to find partial
// results even if the glob search hits permission issues (#24109)
debug('Error in getFilesByGlob %o, retrying with filesystem errors suppressed', e)

return await this.getFilesByGlob(cwd, glob, { ...globOptions, suppressErrors: true })
Copy link
Contributor

@lmiller1990 lmiller1990 Feb 13, 2023

Choose a reason for hiding this comment

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

Should we just do this by default instead of the "if it fails, retry and suppress errors"? Any downside to that approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my original plan, but if we do that we get zero logging of the problem. My fear is that a user will have a spec file within a restricted directory and that spec will just never show up and there will be no logging (debug or otherwise) explaining why which will make it very difficult to troubleshoot.

Taking the "retry with suppress" approach is less performant, but gives us the possibility of debug logs.

}

debug('Non-suppressible error in getFilesByGlob %o', e)

return []
}
Expand Down
29 changes: 29 additions & 0 deletions packages/data-context/test/unit/sources/FileDataSource.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,35 @@ describe('FileDataSource', () => {
},
)
})

it('should retry search with `suppressErrors` if non-suppressed attempt fails', async () => {
matchGlobsStub.onFirstCall().rejects(new Error('mocked filesystem error'))
matchGlobsStub.onSecondCall().resolves(mockMatches)

const files = await fileDataSource.getFilesByGlob(
'/',
'/cypress/e2e/**.cy.js',
{ absolute: false, objectMode: true },
)

expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.callCount(2)
expect(matchGlobsStub.getCall(0).args[1].suppressErrors).to.be.undefined
expect(matchGlobsStub.getCall(1).args[1].suppressErrors).to.equal(true)
})

it('should return empty array if retry with suppression fails', async () => {
matchGlobsStub.rejects(new Error('mocked filesystem error'))

const files = await fileDataSource.getFilesByGlob(
'/',
'/cypress/e2e/**.cy.js',
{ absolute: false, objectMode: true },
)

expect(files).to.eql([])
expect(matchGlobsStub).to.have.callCount(2)
})
})
})
})