Skip to content
Open
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
2 changes: 1 addition & 1 deletion packages/playwright/src/isomorphic/testServerInterface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export interface TestServerInterface {
}>;

runTests(params: {
locations?: string[];
locations: string[];
grep?: string;
grepInvert?: string;
testIds?: string[];
Expand Down
36 changes: 17 additions & 19 deletions packages/playwright/src/isomorphic/testTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,17 +292,6 @@ export class TestTree {
this.rootItem = shortRoot;
}

testIds(): Set<string> {
const result = new Set<string>();
const visit = (treeItem: TreeItem) => {
if (treeItem.kind === 'case')
treeItem.tests.forEach(t => result.add(t.id));
treeItem.children.forEach(visit);
};
visit(this.rootItem);
return result;
}

fileNames(): string[] {
const result = new Set<string>();
const visit = (treeItem: TreeItem) => {
Expand All @@ -329,8 +318,8 @@ export class TestTree {
return this._treeItemById.get(id);
}

collectTestIds(treeItem?: TreeItem): Set<string> {
return treeItem ? collectTestIds(treeItem) : new Set();
collectTestIds(treeItem: TreeItem) {
return collectTestIds(treeItem);
}
}

Expand Down Expand Up @@ -371,18 +360,27 @@ export function sortAndPropagateStatus(treeItem: TreeItem) {
treeItem.status = 'passed';
}

export function collectTestIds(treeItem: TreeItem): Set<string> {
function collectTestIds(treeItem: TreeItem): { testIds: Set<string>, locations: Set<string> } {
const testIds = new Set<string>();
const locations = new Set<string>();
const visit = (treeItem: TreeItem) => {
if (treeItem.kind !== 'test' && treeItem.kind !== 'case') {
treeItem.children.forEach(visit);
return;
}

let fileItem: TreeItem = treeItem;
while (fileItem && fileItem.parent && !(fileItem.kind === 'group' && fileItem.subKind === 'file'))
fileItem = fileItem.parent;
locations.add(fileItem.location.file);

if (treeItem.kind === 'case')
treeItem.tests.map(t => t.id).forEach(id => testIds.add(id));
else if (treeItem.kind === 'test')
testIds.add(treeItem.id);
treeItem.tests.forEach(test => testIds.add(test.id));
else
treeItem.children?.forEach(visit);
testIds.add(treeItem.id);
};
visit(treeItem);
return testIds;
return { testIds, locations };
}

export const statusEx = Symbol('statusEx');
3 changes: 2 additions & 1 deletion packages/playwright/src/mcp/test/testTools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export const runTests = defineTestTool({

handle: async (context, params) => {
const { output } = await context.runTestsWithGlobalSetupAndPossiblePause({
locations: params.locations,
locations: params.locations ?? [],
projects: params.projects,
disableConfigReporters: true,
});
Expand All @@ -74,6 +74,7 @@ export const debugTest = defineTestTool({
handle: async (context, params) => {
const { output, status } = await context.runTestsWithGlobalSetupAndPossiblePause({
headed: context.computedHeaded,
locations: [], // we can make this faster by passing the test's location, so we don't need to scan all tests to find the ID
testIds: [params.test.id],
// For automatic recovery
timeout: 0,
Expand Down
4 changes: 2 additions & 2 deletions packages/playwright/src/runner/testRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export type ListTestsParams = {

export type RunTestsParams = {
timeout?: number;
locations?: string[];
locations: string[];
grep?: string;
grepInvert?: string;
testIds?: string[];
Expand Down Expand Up @@ -331,7 +331,7 @@ export class TestRunner extends EventEmitter<TestRunnerEventMap> {

config.cliListOnly = false;
config.cliPassWithNoTests = true;
config.cliArgs = params.locations || [];
config.cliArgs = params.locations;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not backwards-compatible, so older VSCode extension won't be able to run tests. We should update the extension first.

Copy link
Member Author

Choose a reason for hiding this comment

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

totally agree! #38235 (comment)

config.cliGrep = params.grep;
config.cliGrepInvert = params.grepInvert;
config.cliProjectFilter = params.projects?.length ? params.projects : undefined;
Expand Down
2 changes: 1 addition & 1 deletion packages/playwright/src/runner/watchMode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ async function runTests(watchOptions: WatchModeOptions, testServerConnection: Te
await testServerConnection.runTests({
grep: watchOptions.grep,
testIds: options?.testIds,
locations: watchOptions?.files,
locations: watchOptions?.files ?? [], // TODO: always collect locations based on knowledge about tree, so that we don't have to load all tests
projects: watchOptions.projects,
connectWsEndpoint,
reuseContext: connectWsEndpoint ? true : undefined,
Expand Down
4 changes: 2 additions & 2 deletions packages/trace-viewer/src/ui/uiModeTestListView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const TestListView: React.FC<{
testTree: TestTree,
testServerConnection: TestServerConnection | undefined,
testModel?: TeleSuiteUpdaterTestModel,
runTests: (mode: 'bounce-if-busy' | 'queue-if-busy', testIds: Set<string>) => void,
runTests: (mode: 'bounce-if-busy' | 'queue-if-busy', testTree: TestTree, testItems: Iterable<TreeItem>) => void,
runningState?: { testIds: Set<string>, itemSelectedByUser?: boolean, completed?: boolean },
watchAll: boolean,
watchedTreeIds: { value: Set<string> },
Expand Down Expand Up @@ -139,7 +139,7 @@ export const TestListView: React.FC<{

const runTreeItem = (treeItem: TreeItem) => {
setSelectedTreeItemId(treeItem.id);
runTests('bounce-if-busy', testTree.collectTestIds(treeItem));
runTests('bounce-if-busy', testTree, [treeItem]);
};

const handleTagClick = (e: React.MouseEvent, tag: string) => {
Expand Down
48 changes: 31 additions & 17 deletions packages/trace-viewer/src/ui/uiModeView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,14 @@ if (queryParams.updateSnapshots && !['all', 'changed', 'none', 'missing'].includ

const isMac = navigator.platform === 'MacIntel';

function escapeRegex(text: string) {
// playwright interprets absolute paths as regex,
// removing the leading slash prevents that.
if (text.startsWith('/'))
text = text.substring(1);
return text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
}

export const UIModeView: React.FC<{}> = ({
}) => {
const [filterText, setFilterText] = React.useState<string>('');
Expand All @@ -80,15 +88,14 @@ export const UIModeView: React.FC<{}> = ({
const [testModel, setTestModel] = React.useState<TeleSuiteUpdaterTestModel>();
const [progress, setProgress] = React.useState<TeleSuiteUpdaterProgress & { total: number } | undefined>();
const [selectedItem, setSelectedItem] = React.useState<{ treeItem?: TreeItem, testFile?: SourceLocation, testCase?: reporterTypes.TestCase }>({});
const [visibleTestIds, setVisibleTestIds] = React.useState<Set<string>>(new Set());
const [isLoading, setIsLoading] = React.useState<boolean>(false);
const [runningState, setRunningState] = React.useState<{ testIds: Set<string>, itemSelectedByUser?: boolean, completed?: boolean } | undefined>();
const isRunningTest = runningState && !runningState.completed;

const [watchAll, setWatchAll] = useSetting<boolean>('watch-all', false);
const [watchedTreeIds, setWatchedTreeIds] = React.useState<{ value: Set<string> }>({ value: new Set() });
const commandQueue = React.useRef(Promise.resolve());
const runTestBacklog = React.useRef<Set<string>>(new Set());
const runTestBacklog = React.useRef<{ testIds: Set<string>, locations: Set<string> }>({ testIds: new Set(), locations: new Set() });
const [collapseAllCount, setCollapseAllCount] = React.useState(0);
const [expandAllCount, setExpandAllCount] = React.useState(0);
const [isDisconnected, setIsDisconnected] = React.useState(false);
Expand Down Expand Up @@ -247,20 +254,25 @@ export const UIModeView: React.FC<{}> = ({
testTree.sortAndPropagateStatus();
testTree.shortenRoot();
testTree.flattenForSingleProject();
setVisibleTestIds(testTree.testIds());
return { testTree };
}, [filterText, testModel, statusFilters, projectFilters, setVisibleTestIds, runningState, isRunningTest, mergeFiles]);
}, [filterText, testModel, statusFilters, projectFilters, runningState, isRunningTest, mergeFiles]);

const runTests = React.useCallback((mode: 'queue-if-busy' | 'bounce-if-busy', testIds: Set<string>) => {
const runTests = React.useCallback((mode: 'queue-if-busy' | 'bounce-if-busy', testTree: TestTree, testItems: Iterable<TreeItem>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we pass TestTree here, especially when it differs from testTree declared on the line 249. Can we pass testIds + locations instead? Perhaps introduce a type for this pair.

if (!testServerConnection || !testModel)
return;
if (mode === 'bounce-if-busy' && isRunningTest)
return;

runTestBacklog.current = new Set([...runTestBacklog.current, ...testIds]);
for (const testItem of testItems) {
const { testIds, locations } = testTree.collectTestIds(testItem);
for (const testId of testIds)
runTestBacklog.current.testIds.add(testId);
for (const location of locations)
runTestBacklog.current.locations.add(location);
}
commandQueue.current = commandQueue.current.then(async () => {
const testIds = runTestBacklog.current;
runTestBacklog.current = new Set();
const { testIds, locations } = runTestBacklog.current;
runTestBacklog.current = { testIds: new Set(), locations: new Set() };
if (!testIds.size)
return;

Expand All @@ -282,7 +294,7 @@ export const UIModeView: React.FC<{}> = ({
setRunningState({ testIds });

await testServerConnection.runTests({
locations: queryParams.args,
locations: [...locations].map(escapeRegex),
grep: queryParams.grep,
grepInvert: queryParams.grepInvert,
testIds: [...testIds],
Expand All @@ -302,6 +314,8 @@ export const UIModeView: React.FC<{}> = ({
});
}, [projectFilters, isRunningTest, testModel, testServerConnection, updateSnapshots, singleWorker]);

const runVisibleTests = React.useCallback(() => runTests('bounce-if-busy', testTree, [testTree.rootItem]), [runTests, testTree]);

React.useEffect(() => {
if (!testServerConnection || !teleSuiteUpdater)
return;
Expand All @@ -328,13 +342,13 @@ export const UIModeView: React.FC<{}> = ({
const testModel = teleSuiteUpdater.asModel();
const testTree = new TestTree('', testModel.rootSuite, testModel.loadErrors, projectFilters, queryParams.pathSeparator, mergeFiles);

const testIds: string[] = [];
const testItems = new Set<TreeItem>();
const set = new Set(params.testFiles);
if (watchAll) {
const visit = (treeItem: TreeItem) => {
const fileName = treeItem.location.file;
if (fileName && set.has(fileName))
testIds.push(...testTree.collectTestIds(treeItem));
testItems.add(treeItem);
if (treeItem.kind === 'group' && treeItem.subKind === 'folder')
treeItem.children.forEach(visit);
};
Expand All @@ -344,10 +358,10 @@ export const UIModeView: React.FC<{}> = ({
const treeItem = testTree.treeItemById(treeId);
const fileName = treeItem?.location.file;
if (fileName && set.has(fileName))
testIds.push(...testTree.collectTestIds(treeItem));
testItems.add(treeItem);
}
}
runTests('queue-if-busy', new Set(testIds));
runTests('queue-if-busy', testTree, testItems);
});
return () => disposable.dispose();
}, [runTests, testServerConnection, watchAll, watchedTreeIds, teleSuiteUpdater, projectFilters, mergeFiles]);
Expand All @@ -365,14 +379,14 @@ export const UIModeView: React.FC<{}> = ({
testServerConnection?.stopTestsNoReply({});
} else if (e.code === 'F5') {
e.preventDefault();
runTests('bounce-if-busy', visibleTestIds);
runVisibleTests();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still use visibleTestIds anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, I removed it

}
};
addEventListener('keydown', onShortcutEvent);
return () => {
removeEventListener('keydown', onShortcutEvent);
};
}, [runTests, reloadTests, testServerConnection, visibleTestIds, isShowingOutput]);
}, [runVisibleTests, reloadTests, testServerConnection, isShowingOutput]);

const dialogRef = React.useRef<HTMLDialogElement>(null);
const openInstallDialog = React.useCallback((e: React.MouseEvent) => {
Expand Down Expand Up @@ -456,7 +470,7 @@ export const UIModeView: React.FC<{}> = ({
projectFilters={projectFilters}
setProjectFilters={setProjectFilters}
testModel={testModel}
runTests={() => runTests('bounce-if-busy', visibleTestIds)} />
runTests={runVisibleTests} />
<Toolbar className='section-toolbar' noMinHeight={true}>
{!isRunningTest && !progress && <div className='section-title'>Tests</div>}
{!isRunningTest && progress && <div data-testid='status-line' className='status-line'>
Expand All @@ -465,7 +479,7 @@ export const UIModeView: React.FC<{}> = ({
{isRunningTest && progress && <div data-testid='status-line' className='status-line'>
<div>Running {progress.passed}/{runningState.testIds.size} passed ({(progress.passed / runningState.testIds.size) * 100 | 0}%)</div>
</div>}
<ToolbarButton icon='play' title='Run all — F5' onClick={() => runTests('bounce-if-busy', visibleTestIds)} disabled={isRunningTest || isLoading}></ToolbarButton>
<ToolbarButton icon='play' title='Run all — F5' onClick={runVisibleTests} disabled={isRunningTest || isLoading}></ToolbarButton>
<ToolbarButton icon='debug-stop' title={'Stop — ' + (isMac ? '⇧F5' : 'Shift + F5')} onClick={() => testServerConnection?.stopTests({})} disabled={!isRunningTest || isLoading}></ToolbarButton>
<ToolbarButton icon='eye' title='Watch all' toggled={watchAll} onClick={() => {
setWatchedTreeIds({ value: new Set() });
Expand Down
13 changes: 7 additions & 6 deletions tests/playwright-test/test-server.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class TestServerConnectionUnderTest extends TestServerConnection {
this.onStdio(params => this.events.push(['stdio', params]));
this.onLoadTraceRequested(params => this.events.push(['loadTraceRequested', params]));
this.onTestPaused(params => this.events.push(['testPaused', params]));
this.onReport(params => this.events.push(['report', params]));
}
}

Expand Down Expand Up @@ -166,7 +167,7 @@ test('stdio interception', async ({ startTestServer, writeFiles }) => {
`,
});

const tests = await testServerConnection.runTests({ trace: 'on' });
const tests = await testServerConnection.runTests({ trace: 'on', locations: [] });
expect(tests).toEqual({ status: 'passed' });
await expect.poll(() => testServerConnection.events).toEqual(expect.arrayContaining([
['stdio', { type: 'stderr', text: 'this goes to stderr\n' }],
Expand Down Expand Up @@ -247,7 +248,7 @@ test('timeout override', async ({ startTestServer, writeFiles }) => {
`,
});

expect(await testServerConnection.runTests({ timeout: 42 })).toEqual({ status: 'passed' });
expect(await testServerConnection.runTests({ timeout: 42, locations: [] })).toEqual({ status: 'passed' });
});

test('PLAYWRIGHT_TEST environment variable', async ({ startTestServer, writeFiles }) => {
Expand All @@ -261,7 +262,7 @@ test('PLAYWRIGHT_TEST environment variable', async ({ startTestServer, writeFile
});
`,
});
expect(await testServerConnection.runTests({})).toEqual({ status: 'passed' });
expect(await testServerConnection.runTests({ locations: [] })).toEqual({ status: 'passed' });
});

test('pauseAtEnd', async ({ startTestServer, writeFiles }) => {
Expand All @@ -275,7 +276,7 @@ test('pauseAtEnd', async ({ startTestServer, writeFiles }) => {
`,
});

const promise = testServerConnection.runTests({ pauseAtEnd: true });
const promise = testServerConnection.runTests({ pauseAtEnd: true, locations: [] });
await expect.poll(() => testServerConnection.events.find(e => e[0] === 'testPaused')).toEqual(['testPaused', { errors: [] }]);
await testServerConnection.stopTests({});
expect(await promise).toEqual({ status: 'interrupted' });
Expand All @@ -293,7 +294,7 @@ test('pauseOnError', async ({ startTestServer, writeFiles }) => {
`,
});

const promise = testServerConnection.runTests({ pauseOnError: true });
const promise = testServerConnection.runTests({ pauseOnError: true, locations: [] });
await expect.poll(() => testServerConnection.events.some(e => e[0] === 'testPaused')).toBeTruthy();
expect(testServerConnection.events.find(e => e[0] === 'testPaused')[1]).toEqual({
errors: [
Expand Down Expand Up @@ -324,6 +325,6 @@ test('pauseOnError no errors', async ({ startTestServer, writeFiles }) => {
`,
});

expect(await testServerConnection.runTests({ pauseOnError: true })).toEqual({ status: 'passed' });
expect(await testServerConnection.runTests({ pauseOnError: true, locations: [] })).toEqual({ status: 'passed' });
expect(testServerConnection.events.filter(e => e[0] === 'testPaused')).toEqual([]);
});