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

Ghost testing #2027

Merged
merged 20 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { matchWorkspaces } from '../../utils/getChangedWorkspaces';
import type { WorkspaceContent } from '../../utils/getAllWorkspaces';
import type { WorkspaceContent } from '@modular-scripts/modular-types';

describe('matchWorkspaces', () => {
it('matches absolute manifests paths with holes and duplication to subset of workspace entries', () => {
Expand Down
22 changes: 22 additions & 0 deletions packages/modular-scripts/src/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,20 @@ interface CLITestOptions extends TestOptions {

program
.command('test [regexes...]')
.option(
'--ancestors',
'Additionally run tests for workspaces that depend on workspaces that have changed',
false,
)
.option(
'--debug',
'Setup node.js debugger on the test process - equivalent of setting --inspect-brk on a node.js process',
false,
)
.option(
'--changed <branch>',
Copy link
Contributor

Choose a reason for hiding this comment

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

To allow for the future state of having --changed on its own to use the default / PR head, should this be a boolean with another option of --compare-branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, it'd be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

'Run tests only for workspaces that have changed compared to the branch specified',
)
.option('--coverage', testOptions.coverage.description)
.option('--forceExit', testOptions.forceExit.description)
.option('--env <env>', testOptions.env.description, 'jsdom')
Expand All @@ -155,6 +164,19 @@ program
.allowUnknownOption()
.description('Run tests over the codebase')
.action(async (regexes: string[], options: CLITestOptions) => {
if (options.ancestors && !options.changed) {
process.stderr.write(
"Option --ancestors doesn't make sense without option --changed",
);
process.exit(1);
}
if (options.changed && regexes.length) {
process.stderr.write(
'Option --changed conflicts with supplied test regex',
);
process.exit(1);
}

const { default: test } = await import('./test');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does importing ./test have side-effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case it doesn't need a dynamic import - not something that's part of your PR, but might be easier to follow as a standard import at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@steveukx in program.ts we parse the command line args and we decide which Modular command to execute. If we statically imported all of them at the start (which is translated to sync require in cjs world), it'd be a performance hit proportional to the quantity of commands.


// proxy simplified options to testOptions
Expand Down
59 changes: 56 additions & 3 deletions packages/modular-scripts/src/test/index.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import * as path from 'path';
import { computeAncestorWorkspaces } from '@modular-scripts/workspace-resolver';
import actionPreflightCheck from '../utils/actionPreflightCheck';
import resolve from 'resolve';
import { ExecaError } from 'execa';
import execAsync from '../utils/execAsync';
import getModularRoot from '../utils/getModularRoot';
import { getAllWorkspaces } from '../utils/getAllWorkspaces';
import { getChangedWorkspaces } from '../utils/getChangedWorkspaces';
import { resolveAsBin } from '../utils/resolveAsBin';
import * as logger from '../utils/logger';

export interface TestOptions {
ancestors: boolean;
bail: boolean;
debug: boolean;
changed: string;
clearCache: boolean;
coverage: boolean;
forceExit: boolean;
Expand Down Expand Up @@ -55,9 +60,19 @@ function resolveJestDefaultEnvironment(name: string) {
});
}

async function test(options: TestOptions, regexes?: string[]): Promise<void> {
const { debug, env, reporters, testResultsProcessor, ...jestOptions } =
options;
async function test(
options: TestOptions,
userRegexes?: string[],
): Promise<void> {
const {
ancestors,
changed: changedTargetBranch,
debug,
env,
reporters,
testResultsProcessor,
...jestOptions
} = options;

// create argv from jest Options
const cleanArgv: string[] = [];
Expand Down Expand Up @@ -98,6 +113,22 @@ async function test(options: TestOptions, regexes?: string[]): Promise<void> {
const additionalOptions: string[] = [];
const cleanRegexes: string[] = [];

let regexes = userRegexes;

if (changedTargetBranch) {
const testRegexes = await computeChangedTestsRegexes(
changedTargetBranch,
ancestors,
);
if (!testRegexes.length) {
// No regexes means "run all tests", but we want to avoid that in --changed mode
process.stdout.write('No changed workspaces found');
process.exit(0);
} else {
regexes = testRegexes;
}
}

if (regexes?.length) {
regexes.forEach((reg) => {
if (/^(--)([\w]+)/.exec(reg)) {
Expand Down Expand Up @@ -156,4 +187,26 @@ async function test(options: TestOptions, regexes?: string[]): Promise<void> {
}
}

async function computeChangedTestsRegexes(
targetBranch: string,
ancestors: boolean,
) {
// Get all the workspaces
const allWorkspaces = await getAllWorkspaces(getModularRoot());
// Get the changed workspaces compared to our target branch
const changedWorkspaces = await getChangedWorkspaces(
allWorkspaces,
targetBranch,
);
// Get the ancestors from the changed workspaces
const selectedWorkspaces = ancestors
? computeAncestorWorkspaces(changedWorkspaces, allWorkspaces)
: changedWorkspaces;

const testRegexes = Object.values(selectedWorkspaces[1]).map(
({ location }) => location,
);
return testRegexes;
}

export default actionPreflightCheck(test);
7 changes: 1 addition & 6 deletions packages/modular-scripts/src/utils/getAllWorkspaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,9 @@ import {
} from '@modular-scripts/workspace-resolver';

import type {
WorkspaceContent,
WorkspaceMap,
ModularWorkspacePackage,
} from '@modular-scripts/modular-types';

export type WorkspaceContent = [
Map<string, ModularWorkspacePackage>,
WorkspaceMap,
];
export interface PackageManagerInfo {
getWorkspaceCommand: string;
formatWorkspaceCommandOutput: (stdout: string) => WorkspaceMap;
Expand Down
2 changes: 1 addition & 1 deletion packages/modular-scripts/src/utils/getChangedWorkspaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import path from 'path';
import pkgUp from 'pkg-up';
import { getDiffedFiles } from './gitActions';
import getModularRoot from './getModularRoot';
import type { WorkspaceContent } from './getAllWorkspaces';
import type { WorkspaceContent } from '@modular-scripts/modular-types';

// Gets a list of changed files, then maps them to their workspace and returns a subset of WorkspaceContent
export async function getChangedWorkspaces(
Expand Down
5 changes: 5 additions & 0 deletions packages/modular-types/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ export interface WorkspaceObj extends WorkspaceDependencyObject {

export type WorkspaceMap = Record<string, WorkspaceObj>;

export type WorkspaceContent = [
Map<string, ModularWorkspacePackage>,
WorkspaceMap,
];

// Utility type that extends type `T1` with the fields of type `T2`
type Extend<T1, T2> = {
[k in keyof (T1 & T2)]: k extends keyof T2
Expand Down
7 changes: 1 addition & 6 deletions packages/workspace-resolver/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,4 @@ export {
analyzeWorkspaceDependencies,
} from './resolve-workspace';

export {
computeAncestorSet,
computeDescendantSet,
traverseWorkspaceRelations,
invertDependencyDirection,
} from './resolve-dependencies';
export * from './resolve-dependencies';
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
import type { WorkspaceDependencyObject } from '@modular-scripts/modular-types';

type OrderedDependencies = Map<string, number>;
type OrderedUnvisited = { name: string; level: number };
type OptionalWorkspaceDependencyObject = Partial<WorkspaceDependencyObject>;

export function computeDescendantSet(
workspaceNames: string[],
allWorkspaces: Record<string, OptionalWorkspaceDependencyObject>,
): Set<string> {
// Really simple and performant ancestors collection: walk the graph via BFS and collect all the dependencies encountered in a set.
// If one dependency was already encountered before, don't process it. This is cycle resistant.
const unvisited: string[] = [...workspaceNames];
const visited: Set<string> = new Set();

while (unvisited.length) {
const currentDependency = unvisited.shift();
if (!currentDependency) break;
visited.add(currentDependency);

const immediateDependencies =
allWorkspaces[currentDependency]?.workspaceDependencies;

if (immediateDependencies) {
for (const immediateDep of immediateDependencies) {
if (!visited.has(immediateDep)) {
unvisited.push(immediateDep);
}
}
}
}
return setDiff(visited, new Set(workspaceNames));
}

export function computeAncestorSet(
workspaceNames: string[],
allWorkspaces: Record<string, WorkspaceDependencyObject>,
): Set<string> {
// Computing an ancestor set is like computing a dependant set with an inverted graph
return computeDescendantSet(
workspaceNames,
invertDependencyDirection(allWorkspaces),
);
}

// This function takes a tree of dependencies (dependant -> child dependencies)
// and returns an equivalent tree where the relation's direction is inverted
// (dependency -> parent dependencies)
// This allows us to use the same algorithm to query ancestors or descendants.
export function invertDependencyDirection(
workspaces: Record<string, WorkspaceDependencyObject>,
): Record<string, OptionalWorkspaceDependencyObject> {
return Object.entries(workspaces).reduce<
Record<string, WorkspaceDependencyObject>
>((output, [currentWorkspace, workspaceRecord]) => {
// Loop through all the dependencies for currentWorkspace and save the inverse relation in the output
workspaceRecord.workspaceDependencies?.forEach((dependency) => {
// Create a workspaceAncestors record if not already present
if (!output[dependency]) {
output[dependency] = { workspaceDependencies: [] };
}
// Insert if the ancestor is not already present.
// This would be less costly with a Set, but a Set would come at the cost of arrayfy-ing all the Sets later
if (
!output[dependency].workspaceDependencies?.includes(currentWorkspace)
) {
output[dependency].workspaceDependencies?.push(currentWorkspace);
}
});
return output;
}, Object.create(null));
}

// This function traverses the graph to get an ordered set of dependencies (map reverseOrder => dependencyName)
export function traverseWorkspaceRelations(
workspaceNames: string[],
workspaces: Record<string, WorkspaceDependencyObject>,
): OrderedDependencies {
const workspaceN = Object.values(workspaces).length;
// Initialize the unvisited list with the immediate dependency arrays.
const unvisitedList = workspaceNames.reduce<string[]>(
(acc, workspaceName) => {
const immediate = workspaces[workspaceName]?.workspaceDependencies ?? [];
return [...acc, ...immediate];
},
[],
);
// Dedupe initial unvisited and start from order 1
const unvisited: OrderedUnvisited[] = Array.from(new Set(unvisitedList)).map(
(dep) => ({
name: dep,
level: 1,
}),
);

// visited holds all the nodes that we've visited previously
const visited: OrderedDependencies = new Map(
workspaceNames.map((dep) => [dep, 0]),
);
// cycleBreaker holds our DFS path and helps identifying cycles
const cycleBreaker: Set<string> = new Set();

while (unvisited.length) {
// Consume the remaining unvisited descendants one by one
const unvisitedDependency = unvisited.shift();
if (!unvisitedDependency) break;

const { name: currentDependencyName, level: currentDependencyDepth } =
unvisitedDependency;
cycleBreaker.add(currentDependencyName);

// Get the next immediate dependencies of the dependency we're visiting.
const immediateDependencies =
workspaces[currentDependencyName]?.workspaceDependencies;

// Add current dependency to the visited set.
// If we already visited it at a lower depth in the graph, raise its level to the current depth
// (i.e. this dependency could be a dependency of some other node, but since is also a dependency of *this* node, it gets the bigger depth of the two)
const dependencyLevel = visited.has(currentDependencyName)
? Math.max(
currentDependencyDepth,
visited.get(currentDependencyName) ?? -1,
)
: currentDependencyDepth;
visited.set(currentDependencyName, dependencyLevel);

// All our immediate dependencies are inserted into unvisited, with a depth level = this node + 1
if (immediateDependencies) {
const immediateDependenciesWithDepth = immediateDependencies?.map(
(dep) => ({
name: dep,
level: currentDependencyDepth + 1,
}),
);
for (const dep of immediateDependenciesWithDepth) {
// If we're enqueueing a dependency that we have already processed in this walk, we have a cycle.
if (cycleBreaker.has(dep.name) || currentDependencyDepth > workspaceN) {
throw new Error(
`Cycle detected, ${[...cycleBreaker, dep.name].join(' -> ')}`,
);
}
// If we insert the immediate dependencies at the end (push), we do a BFS walk.
// If we insert them at the start (unshift), we do a DFS walk.
unvisited.unshift(dep);
}

// If we got to an end node, we finish the current DFS traversal: reset the cycle breaker
if (!immediateDependencies || !immediateDependencies.length) {
cycleBreaker.clear();
}
}
}

return visited;
}

function setDiff<T>(a: Set<T>, b: Set<T>): Set<T> {
return new Set([...a].filter((x) => !b.has(x)));
}

export function setUnion<T>(
a: Set<T> | Array<T>,
b: Set<T> | Array<T>,
): Set<T> {
return new Set([...a, ...b]);
}
Loading