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

Selective test on workspace name with the --package option #2145

Merged
merged 8 commits into from
Sep 20, 2022
Merged
Show file tree
Hide file tree
Changes from 7 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
5 changes: 5 additions & 0 deletions .changeset/friendly-dancers-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"modular-scripts": minor
---

Selective test on workspace name with the --package option
25 changes: 17 additions & 8 deletions docs/commands/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ Specify the comparison branch used to determine which files have changed when
using the `changed` option. If this option is used without `changed`, the
command will fail.

#### collectCoverageFrom
### collectCoverageFrom

[_Documentation_](https://jestjs.io/docs/configuration#collectcoveragefrom-array)

Expand All @@ -88,7 +88,7 @@ Example:
}
```

#### coveragePathIgnorePatterns
### coveragePathIgnorePatterns

[_Documentation_](https://jestjs.io/docs/configuration#coveragepathignorepatterns-arraystring)

Expand All @@ -98,7 +98,7 @@ An array of regexp pattern strings that are matched against all file paths
before executing the test. If the file path matches any of the patterns,
coverage information will be skipped.

#### coverageThreshold
### coverageThreshold

[_Documentation_](https://jestjs.io/docs/configuration#coveragethreshold-object)

Expand All @@ -111,7 +111,7 @@ positive number are taken to be the minimum percentage required. Thresholds
specified as a negative number represent the maximum number of uncovered
entities allowed.

#### moduleNameMapper
### moduleNameMapper

[_Documentation_](https://jestjs.io/docs/configuration#modulenamemapper-objectstring-string--arraystring)

Expand All @@ -131,7 +131,7 @@ The moduleNameMapper is merged with the `modular` defaults to provide common use
cases for static assets, like static assets including images, CSS and
CSS-modules.

#### modulePathIgnorePatterns
### modulePathIgnorePatterns

[_Documentation_](https://jestjs.io/docs/configuration#modulepathignorepatterns-arraystring)

Expand All @@ -142,7 +142,16 @@ before those paths are to be considered 'visible' to the module loader. If a
given module's path matches any of the patterns, it will not be `require()`-able
in the test environment.

#### testPathIgnorePatterns
### package

Default: `undefined`

Run all the test for the workspace with the specified package name. Can be
cristiano-belloni marked this conversation as resolved.
Show resolved Hide resolved
repeated to select more than one workspace. Can be combined with the
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a typo?

Suggested change
repeated to select more than one workspace. Can be combined with the
repeated to select more than one package. Can be combined with the

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not. I'm confused - are they workspaces or packages? In my mind, a workspace can consist of many packages (the workspace usually means the packages dir, if talking in yarn languge)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's incredibly confusing, but it seems that any package that falls in the workspaces glob / array in the manifest is called a "workspace": https://classic.yarnpkg.com/lang/en/docs/workspaces/

`--ancestors` option to test the specified workspace(s) plus all the workspaces
that, directly or indirectly, depend on them. Conflicts with `--changed`.

### testPathIgnorePatterns

[_Documentation_](https://jestjs.io/docs/configuration#testpathignorepatterns-arraystring)

Expand All @@ -152,7 +161,7 @@ An array of regexp pattern strings that are matched against all test paths
before executing the test. If the test path matches any of the patterns, it will
be skipped.

#### testRunner
### testRunner

[_Documentation_](https://jestjs.io/docs/configuration#testrunner-string)

Expand All @@ -166,7 +175,7 @@ This can be used to revert to the previous `jest` default testRunner
(`jest-jasmine2`) in cases where `circus` is not yet compatible with an older
codebase.

#### transformIgnorePatterns
### transformIgnorePatterns

[_Documentation_](https://jestjs.io/docs/configuration#transformignorepatterns-arraystring)

Expand Down
203 changes: 203 additions & 0 deletions packages/modular-scripts/src/__tests__/test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,209 @@ describe('Modular test command', () => {
);
});
});

describe('test command can successfully do selective tests based on selected packages', () => {
const fixturesFolder = path.join(
__dirname,
Array.from({ length: 4 }).reduce<string>(
(acc) => `${acc}..${path.sep}`,
'',
),
'__fixtures__',
'ghost-testing',
);

const currentModularFolder = getModularRoot();
let randomOutputFolder: string;

beforeEach(() => {
// Create random dir
randomOutputFolder = tmp.dirSync({ unsafeCleanup: true }).name;
fs.copySync(fixturesFolder, randomOutputFolder);
execa.sync('yarn', {
cwd: randomOutputFolder,
});
});

// Run in a single test, serially for performance reasons (the setup time is quite long)
it('finds --package after specifying a valid workspaces / finds ancestors using --ancestors', () => {
const resultPackages = runRemoteModularTest(
currentModularFolder,
randomOutputFolder,
['test', '--package', 'b', '--package', 'c'],
);
expect(resultPackages.stderr).toContain(
'packages/c/src/__tests__/utils/c-nested.test.ts',
);
expect(resultPackages.stderr).toContain(
'packages/c/src/__tests__/c.test.ts',
);
expect(resultPackages.stderr).toContain(
'packages/b/src/__tests__/utils/b-nested.test.ts',
);
expect(resultPackages.stderr).toContain(
'packages/b/src/__tests__/b.test.ts',
);

const resultPackagesWithAncestors = runRemoteModularTest(
currentModularFolder,
randomOutputFolder,
['test', '--ancestors', '--package', 'b', '--package', 'c'],
);
expect(resultPackagesWithAncestors.stderr).toContain(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could maybe use a single expect statement here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmh, not sure what you mean - it's not possible to pass an array to toContain and it's not possible to chain expectations, how would you use a single expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you're saying that we can conflate packages/c/src/__tests__/utils/c-nested.test.ts and packages/c/src/__tests__/utils/c-nested.test.ts into their common root: packages/c/src/__tests__/; in that case I explicitly wanted to show that we're executing all the tests of a workspace, including those in subdirectories.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the shape of resultPackagesWithAncestors.stderr? I can imagine we can find a way to dedupe this a bit, but it's really not a big deal

'packages/c/src/__tests__/utils/c-nested.test.ts',
);
expect(resultPackagesWithAncestors.stderr).toContain(
'packages/c/src/__tests__/c.test.ts',
);
expect(resultPackagesWithAncestors.stderr).toContain(
'packages/b/src/__tests__/utils/b-nested.test.ts',
);
expect(resultPackagesWithAncestors.stderr).toContain(
'packages/b/src/__tests__/b.test.ts',
);
expect(resultPackagesWithAncestors.stderr).toContain(
'packages/a/src/__tests__/utils/a-nested.test.ts',
);
expect(resultPackagesWithAncestors.stderr).toContain(
'packages/a/src/__tests__/a.test.ts',
);
expect(resultPackagesWithAncestors.stderr).toContain(
'packages/e/src/__tests__/utils/e-nested.test.ts',
);
expect(resultPackagesWithAncestors.stderr).toContain(
'packages/e/src/__tests__/e.test.ts',
);
});
});

describe('test command has error states', () => {
// Run in a single test, serially for performance reasons (the setup time is quite long)
it('errors when specifying --package with --changed', async () => {
let errorNumber;
try {
await execa(
'yarnpkg',
['modular', 'test', '--changed', '--package', 'modular-scripts'],
{
all: true,
cleanup: true,
},
);
} catch (error) {
errorNumber = (error as ExecaError).exitCode;
}
expect(errorNumber).toEqual(1);
});

it('errors when specifying --package with a non-existing workspace', async () => {
let capturedError;
try {
await execa(
'yarnpkg',
['modular', 'test', '--package', 'non-existing'],
{
all: true,
cleanup: true,
},
);
} catch (error) {
capturedError = error as ExecaError;
}
expect(capturedError?.exitCode).toEqual(1);
expect(capturedError?.stderr).toContain(
`Package non-existing was specified, but Modular couldn't find it`,
);
});

it('errors when specifying a regex with --packages', async () => {
let capturedError;
try {
await execa(
'yarnpkg',
[
'modular',
'test',
'memoize.test.ts',
'--package',
'modular-scripts',
],
{
all: true,
cleanup: true,
},
);
} catch (error) {
capturedError = error as ExecaError;
}
expect(capturedError?.exitCode).toEqual(1);
expect(capturedError?.stderr).toContain(
`Option --package conflicts with supplied test regex`,
);
});

it('errors when specifying a regex with --package', async () => {
let capturedError;
try {
await execa(
'yarnpkg',
[
'modular',
'test',
'memoize.test.ts',
'--package',
'modular-scripts',
],
{
all: true,
cleanup: true,
},
);
} catch (error) {
capturedError = error as ExecaError;
}
expect(capturedError?.exitCode).toEqual(1);
expect(capturedError?.stderr).toContain(
`Option --package conflicts with supplied test regex`,
);
});

it('errors when specifying a regex with --changed', async () => {
let capturedError;
try {
await execa(
'yarnpkg',
['modular', 'test', 'memoize.test.ts', '--changed'],
{
all: true,
cleanup: true,
},
);
} catch (error) {
capturedError = error as ExecaError;
}
expect(capturedError?.exitCode).toEqual(1);
expect(capturedError?.stderr).toContain(
`Option --changed conflicts with supplied test regex`,
);
});

it('errors when specifying --compareBranch without --changed', async () => {
let capturedError;
try {
await execa('yarnpkg', ['modular', 'test', '--compareBranch', 'main'], {
all: true,
cleanup: true,
});
} catch (error) {
capturedError = error as ExecaError;
}
expect(capturedError?.exitCode).toEqual(1);
expect(capturedError?.stderr).toContain(
`Option --compareBranch doesn't make sense without option --changed`,
);
});
});
});

function runRemoteModularTest(
Expand Down
21 changes: 17 additions & 4 deletions packages/modular-scripts/src/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ program
'--compareBranch <branch>',
"Specifies the branch to use with the --changed flag. If not specified, Modular will use the repo's default branch",
)
.option('--package <packages...>', 'Specifies one or more packages to test')
.option('--coverage', testOptions.coverage.description)
.option('--forceExit', testOptions.forceExit.description)
.option('--env <env>', testOptions.env.description, 'jsdom')
Expand All @@ -168,21 +169,33 @@ program
.allowUnknownOption()
.description('Run tests over the codebase')
.action(async (regexes: string[], options: CLITestOptions) => {
if (options.ancestors && !options.changed) {
if (options.ancestors && !options.changed && !options.package) {
process.stderr.write(
"Option --ancestors doesn't make sense without option --changed",
"Option --ancestors doesn't make sense without option --changed or option --package\n",
);
process.exit(1);
}
if (options.package && options.changed) {
process.stderr.write(
'Option --package conflicts with option --changed\n',
);
process.exit(1);
}
if (options.compareBranch && !options.changed) {
process.stderr.write(
"Option --compareBranch doesn't make sense without option --changed",
"Option --compareBranch doesn't make sense without option --changed\n",
);
process.exit(1);
}
if (options.changed && regexes.length) {
process.stderr.write(
'Option --changed conflicts with supplied test regex',
'Option --changed conflicts with supplied test regex\n',
);
process.exit(1);
}
if (options.package && regexes.length) {
process.stderr.write(
'Option --package conflicts with supplied test regex\n',
);
process.exit(1);
}
Expand Down
Loading