Skip to content
Open
Show file tree
Hide file tree
Changes from 5 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
25 changes: 17 additions & 8 deletions packages/playwright/src/isomorphic/testTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,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 +371,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>(); // TODO
const visit = (treeItem: TreeItem) => {
if (treeItem.kind !== 'test' && treeItem.kind !== 'case') {
treeItem.children.forEach(visit);
return;
}

let file: TreeItem = treeItem;
while (file && file.parent && !(file.kind === 'group' && file.subKind === 'file'))
file = file.parent;
locations.add(file.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
2 changes: 1 addition & 1 deletion packages/playwright/src/runner/testRunner.ts
Original file line number Diff line number Diff line change
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 ?? ['dont-match-anything']; // for performance reasons, we require test runner users to pass `locations`. they can opt out by passing [].
config.cliGrep = params.grep;
config.cliGrepInvert = params.grepInvert;
config.cliProjectFilter = params.projects?.length ? params.projects : undefined;
Expand Down
5 changes: 3 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', testIds: Set<string>, locations: Set<string>) => void,
runningState?: { testIds: Set<string>, itemSelectedByUser?: boolean, completed?: boolean },
watchAll: boolean,
watchedTreeIds: { value: Set<string> },
Expand Down Expand Up @@ -139,7 +139,8 @@ export const TestListView: React.FC<{

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

const handleTagClick = (e: React.MouseEvent, tag: string) => {
Expand Down
50 changes: 35 additions & 15 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 @@ -88,7 +96,7 @@ export const UIModeView: React.FC<{}> = ({
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 @@ -251,16 +259,19 @@ export const UIModeView: React.FC<{}> = ({
return { testTree };
}, [filterText, testModel, statusFilters, projectFilters, setVisibleTestIds, 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', testIds: Set<string>, locations: Set<string>) => {
if (!testServerConnection || !testModel)
return;
if (mode === 'bounce-if-busy' && isRunningTest)
return;

runTestBacklog.current = new Set([...runTestBacklog.current, ...testIds]);
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 +293,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 +313,8 @@ export const UIModeView: React.FC<{}> = ({
});
}, [projectFilters, isRunningTest, testModel, testServerConnection, updateSnapshots, singleWorker]);

const runVisibleTests = React.useCallback(() => runTests('bounce-if-busy', visibleTestIds, new Set(queryParams.args)), [runTests, visibleTestIds]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how queryParams.args integrates here. Could you please explain? I'd assume we always use collectTestIds() when running tests, and use queryParams.args when listing to filter things, but I'm not exactly sure.

Copy link
Member Author

@Skn0tt Skn0tt Nov 18, 2025

Choose a reason for hiding this comment

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

i wanted to make it as much of a mechanical refactoring as possible, so I didn't think too closely about the meaning here. We passed queryParams.args into locations before, and we used visibleTestIds here for reasons unbeknownst to me. So I kept it the same.

I refactored it now and was able to remove a bunch of code, so that's great!


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

const testIds: string[] = [];
const locations: string[] = [];
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));
if (fileName && set.has(fileName)) {
const { testIds: ids, locations: locs } = testTree.collectTestIds(treeItem);
testIds.push(...ids);
locations.push(...locs);
}
if (treeItem.kind === 'group' && treeItem.subKind === 'folder')
treeItem.children.forEach(visit);
};
Expand All @@ -343,11 +360,14 @@ export const UIModeView: React.FC<{}> = ({
for (const treeId of watchedTreeIds.value) {
const treeItem = testTree.treeItemById(treeId);
const fileName = treeItem?.location.file;
if (fileName && set.has(fileName))
testIds.push(...testTree.collectTestIds(treeItem));
if (fileName && set.has(fileName)) {
const { testIds: ids, locations: locs } = testTree.collectTestIds(treeItem);
testIds.push(...ids);
locations.push(...locs);
}
}
}
runTests('queue-if-busy', new Set(testIds));
runTests('queue-if-busy', new Set(testIds), new Set(locations));
});
return () => disposable.dispose();
}, [runTests, testServerConnection, watchAll, watchedTreeIds, teleSuiteUpdater, projectFilters, mergeFiles]);
Expand All @@ -365,14 +385,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 +476,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 +485,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([]);
});
Loading