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.

Sign up for GitHub

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
Merged

fix: filtering with --spec option for projects in Docker container root #23535

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e2838a
fix: updating globbing patterns to work when mounted to the root dire…
tbiethman Aug 16, 2022
645e916
Adding unit tests for globby calls
tbiethman Aug 17, 2022
77d87aa
Updating a few tests. Reordering for cleaner diff.
tbiethman Aug 24, 2022
070e002
Merge branch 'develop' of https://github.com/cypress-io/cypress into …
tbiethman Aug 24, 2022
806b77e
Refactoring after develop merge
tbiethman Aug 24, 2022
d7e3679
Persist binary for validation
tbiethman Aug 25, 2022
e0d4af7
Merge branch 'develop' into tbiethman/23380-root-spec-pattern
tbiethman Aug 25, 2022
8109aae
Using arch-specific path sep
tbiethman Aug 29, 2022
e15581b
Merge branch 'develop' into tbiethman/23380-root-spec-pattern
tbiethman Aug 29, 2022
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
12 changes: 9 additions & 3 deletions packages/data-context/src/sources/FileDataSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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.


export class FileDataSource {
constructor (private ctx: DataContext) {}

Expand All @@ -32,12 +36,14 @@ export class FileDataSource {

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

// If the pattern includes the working directory, we strip it from the pattern.
// The working directory path may include characters that conflict with glob
// syntax (brackets, parentheses, etc.) and cause our searches to inadvertently fail.
// We scope our search to the working directory using the `cwd` globby option.
if (globPattern.startsWith(cwd)) {
return globPattern.replace(cwd, '.')
if (globPattern.startsWith(workingDirectoryPrefix)) {
return globPattern.replace(workingDirectoryPrefix, '')
}

return globPattern
Comment on lines +45 to 49
Copy link
Contributor

@flotwig flotwig Aug 29, 2022 •
edited
Loading

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.

Expand All @@ -62,7 +68,7 @@ export class FileDataSource {
debug('globbing pattern(s): %o', globs)
debug('within directory: %s', cwd)

const files = await globby(globs, { onlyFiles: true, absolute: true, cwd, ...globOptions, ignore: ignoreGlob })
const files = await matchGlobs(globs, { onlyFiles: true, absolute: true, cwd, ...globOptions, ignore: ignoreGlob })

return files
} catch (e) {
Expand Down
324 changes: 235 additions & 89 deletions packages/data-context/test/unit/sources/FileDataSource.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,127 +5,273 @@ import path from 'path'
import os from 'os'

import { scaffoldProject, removeProject, createTestDataContext } from '../helper'
import { FileDataSource } from '../../../src/sources/FileDataSource'
import * as FileDataSourceModule from '../../../src/sources/FileDataSource'
import { DataContext } from '../../../src'
import * as fileUtil from '../../../src/util/file'

const FileDataSource = FileDataSourceModule.FileDataSource

describe('FileDataSource', () => {
let projectPath: string
let scriptsFolder: string
let ctx: DataContext
let fileDataSource: FileDataSource
describe('#getFilesByGlob', () => {
describe('integration', () => {
let projectPath: string
let scriptsFolder: string
let ctx: DataContext
let fileDataSource

beforeEach(async () => {
projectPath = await scaffoldProject('globby-test-bed')
scriptsFolder = path.join(projectPath, 'scripts')
beforeEach(async () => {
projectPath = await scaffoldProject('globby-test-bed')
scriptsFolder = path.join(projectPath, 'scripts')

ctx = createTestDataContext('open')
ctx.coreData.currentTestingType = 'e2e'
ctx = createTestDataContext('open')
ctx.coreData.currentTestingType = 'e2e'

fileDataSource = new FileDataSource(ctx)
})
fileDataSource = new FileDataSource(ctx)
})

afterEach(() => {
removeProject('globby-test-bed')
sinon.restore()
})
afterEach(() => {
removeProject('globby-test-bed')
sinon.restore()
})

describe('#getFilesByGlob', () => {
it('finds files at root matching given pattern using globby', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
'root-script-*.js',
)
describe('#getFilesByGlob', () => {
it('finds files at root matching given pattern using globby', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
'root-script-*.js',
)

expect(files).to.have.length(2)
})
expect(files).to.have.length(2)
expect(files[0]).to.eq(path.join(projectPath, 'root-script-1.js'))
expect(files[1]).to.eq(path.join(projectPath, 'root-script-2.js'))
})

it('finds files matching relative patterns in working dir', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
'./root-script-*.js',
)
it('finds files matching relative patterns in working dir', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
'./root-script-*.js',
)

expect(files).to.have.length(2)
})
expect(files).to.have.length(2)
})

it('finds files matching patterns that include working dir', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
`${projectPath}/root-script-*.js`,
)
it('finds files matching patterns that include working dir', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
`${projectPath}/root-script-*.js`,
)

expect(files).to.have.length(2)
})
expect(files).to.have.length(2)
})

it('does not replace working directory in glob pattern if it is not leading', async () => {
// Create a redundant structure within the project dir matching its absolute path
// and write a new script in that location
const nestedScriptPath = path.join(projectPath, 'cypress', projectPath)
it('does not replace working directory in glob pattern if it is not leading', async () => {
// Create a redundant structure within the project dir matching its absolute path
// and write a new script in that location
const nestedScriptPath = path.join(projectPath, 'cypress', projectPath)

await fs.mkdirs(nestedScriptPath)
await fs.writeFile(path.join(nestedScriptPath, 'nested-script.js'), '')
await fs.mkdirs(nestedScriptPath)
await fs.writeFile(path.join(nestedScriptPath, 'nested-script.js'), '')

// Verify that the glob pattern is not impacted if if contains directories equivalent
// to the working directory
let files = await fileDataSource.getFilesByGlob(
projectPath,
`./cypress${projectPath}/nested-script.js`,
)
// Verify that the glob pattern is not impacted if if contains directories equivalent
// to the working directory
let files = await fileDataSource.getFilesByGlob(
projectPath,
`./cypress${projectPath}/nested-script.js`,
)

expect(files).to.have.length(1)
})
expect(files).to.have.length(1)
})

it('finds files matching multiple patterns', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
['root-script-*.js', 'scripts/**/*.js'],
)
it('finds files matching multiple patterns', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
['root-script-*.js', 'scripts/**/*.js'],
)

expect(files).to.have.length(5)
})
expect(files).to.have.length(5)
})

it('does not find files outside of working dir', async () => {
const files = await fileDataSource.getFilesByGlob(
scriptsFolder,
['root-script-*.js', './**/*.js'],
)
it('does not find files outside of working dir', async () => {
const files = await fileDataSource.getFilesByGlob(
scriptsFolder,
['root-script-*.js', './**/*.js'],
)

expect(files).to.have.length(3)
})
expect(files).to.have.length(3)
})

it('always ignores files within node_modules', async () => {
const nodeModulesPath = path.join(projectPath, 'node_modules')

await fs.mkdir(nodeModulesPath)
await fs.writeFile(path.join(nodeModulesPath, 'module-script-1.js'), '')
await fs.writeFile(path.join(nodeModulesPath, 'module-script-2.js'), '')

const files = await fileDataSource.getFilesByGlob(
projectPath,
'**/*script-*.js',
{ ignore: ['./scripts/**/*'] },
)

// only scripts at root should be found, as node_modules is implicitly ignored
// and ./scripts is explicitly ignored
expect(files).to.have.length(2)
})

it('always ignores files within node_modules', async () => {
const nodeModulesPath = path.join(projectPath, 'node_modules')
it('converts globs to POSIX paths on windows', async () => {
const windowsSeperator = '\\'

await fs.mkdir(nodeModulesPath)
await fs.writeFile(path.join(nodeModulesPath, 'module-script-1.js'), '')
await fs.writeFile(path.join(nodeModulesPath, 'module-script-2.js'), '')
sinon.stub(os, 'platform').returns('win32')
const toPosixStub = sinon.stub(fileUtil, 'toPosix').callsFake((path) => {
return toPosixStub.wrappedMethod(path, windowsSeperator)
})

const files = await fileDataSource.getFilesByGlob(
projectPath,
'**/*script-*.js',
{ ignore: ['./scripts/**/*'] },
)
const files = await fileDataSource.getFilesByGlob(
projectPath,
`**${windowsSeperator}*script-*.js`,
)

// only scripts at root should be found, as node_modules is implicitly ignored
// and ./scripts is explicitly ignored
expect(files).to.have.length(2)
expect(files).to.have.length(5)
})

it('finds files using given globby options', async () => {
const files = await fileDataSource.getFilesByGlob(
projectPath,
'root-script-*.js',
{ absolute: false },
)

expect(files).to.have.length(2)
expect(files[0]).to.eq('root-script-1.js')
expect(files[1]).to.eq('root-script-2.js')
})
})
})

it('converts globs to POSIX paths on windows', async () => {
const windowsSeperator = '\\'
describe('unit', () => {
const ctx = createTestDataContext('open')

ctx.coreData.currentTestingType = 'e2e'

const fileDataSource = new FileDataSource(ctx)
const mockMatches = ['/mock/matches']
const defaultGlobbyOptions = {
onlyFiles: true,
absolute: true,
ignore: ['**/node_modules/**'],
}

sinon.stub(os, 'platform').returns('win32')
const toPosixStub = sinon.stub(fileUtil, 'toPosix').callsFake((path) => {
return toPosixStub.wrappedMethod(path, windowsSeperator)
let matchGlobsStub: sinon.SinonStub

beforeEach(() => {
matchGlobsStub = sinon.stub(FileDataSourceModule, 'matchGlobs').resolves(mockMatches)
})

afterEach(() => {
sinon.restore()
})

it('matches absolute patterns when working directory is root', async () => {
const files = await fileDataSource.getFilesByGlob(
'/',
'/cypress/e2e/**.cy.js',
)

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.

{ ...defaultGlobbyOptions, cwd: '/' },
)
})

it('matches relative patterns when working directory is root', async () => {
const files = await fileDataSource.getFilesByGlob(
'/',
'./project/**.cy.js',
)

expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.been.calledWith(
['./project/**.cy.js'],
{ ...defaultGlobbyOptions, cwd: '/' },
)
})

it('matches implicit relative patterns when working directory is root', async () => {
const files = await fileDataSource.getFilesByGlob(
'/',
'project/**.cy.js',
)

expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.been.calledWith(
['project/**.cy.js'],
{ ...defaultGlobbyOptions, cwd: '/' },
)
})

it('matches absolute patterns without including working dir in pattern', async () => {
const files = await fileDataSource.getFilesByGlob(
'/my/project',
'/my/project/cypress/e2e/**.cy.js',
)

expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.been.calledWith(
['cypress/e2e/**.cy.js'],
{ ...defaultGlobbyOptions, cwd: '/my/project' },
)
})

const files = await fileDataSource.getFilesByGlob(
projectPath,
`**${windowsSeperator}*script-*.js`,
)
it('matches absolute patterns that include a copy of the working dir structure', async () => {
const files = await fileDataSource.getFilesByGlob(
'/my/project',
'/my/project/cypress/my/project/e2e/**.cy.js',
)

expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.been.calledWith(
['cypress/my/project/e2e/**.cy.js'],
{ ...defaultGlobbyOptions, cwd: '/my/project' },
)
})

expect(files).to.have.length(5)
it('uses supplied ignore option in conjunction with defaults', async () => {
const files = await fileDataSource.getFilesByGlob(
'/',
'/cypress/e2e/**.cy.js',
{ ignore: ['ignore/foo.*', '/ignore/bar.*'] },
)

expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.been.calledWith(
['cypress/e2e/**.cy.js'],
{
...defaultGlobbyOptions,
cwd: '/',
ignore: ['ignore/foo.*', '/ignore/bar.*', ...defaultGlobbyOptions.ignore],
},
)
})

it('uses supplied globby options', async () => {
const files = await fileDataSource.getFilesByGlob(
'/',
'/cypress/e2e/**.cy.js',
{ absolute: false, objectMode: true },
)

expect(files).to.eq(mockMatches)
expect(matchGlobsStub).to.have.been.calledWith(
['cypress/e2e/**.cy.js'],
{
...defaultGlobbyOptions,
cwd: '/',
absolute: false,
objectMode: true,
},
)
})
})
})
})