Skip to content

Commit

Permalink
per file discovery on save
Browse files Browse the repository at this point in the history
  • Loading branch information
eleanorjboyd committed Dec 1, 2023
1 parent 5302d0e commit 7fe16ee
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 9 deletions.
15 changes: 14 additions & 1 deletion src/client/testing/testController/common/resultResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,18 @@ export class PythonResultResolver implements ITestResultResolver {
);
errorNodeLabel.isTrusted = true;
errorNode.error = errorNodeLabel;
} else if (rawTestData.status === 'error-empty') {
// search this.testController.items find item with uri and delete it
if (rawTestData.error) {
const uri = rawTestData.error[0];
this.testController.items.forEach((item) => {
item.children.forEach((child) => {
if (child.id === uri) {
item.children.delete(child.id);
}
});
});
}
} else {
// remove error node only if no errors exist.
this.testController.items.delete(`DiscoveryError:${workspacePath}`);
Expand All @@ -102,7 +114,8 @@ export class PythonResultResolver implements ITestResultResolver {

// If the test root for this folder exists: Workspace refresh, update its children.
// Otherwise, it is a freshly discovered workspace, and we need to create a new test root and populate the test tree.
populateTestTree(this.testController, rawTestData.tests, undefined, this, token);
const node = this.testController.items.get(workspacePath);
populateTestTree(this.testController, rawTestData.tests, node, this, token);
}

sendTelemetryEvent(EventName.UNITTEST_DISCOVERY_DONE, undefined, {
Expand Down
4 changes: 2 additions & 2 deletions src/client/testing/testController/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ export interface ITestResultResolver {
export interface ITestDiscoveryAdapter {
// ** first line old method signature, second line new method signature
discoverTests(uri: Uri): Promise<DiscoveredTestPayload>;
discoverTests(uri: Uri, executionFactory: IPythonExecutionFactory): Promise<DiscoveredTestPayload>;
discoverTests(uri: Uri, executionFactory: IPythonExecutionFactory, fileUri?: Uri): Promise<DiscoveredTestPayload>;
}

// interface for execution/runner adapter
Expand Down Expand Up @@ -251,7 +251,7 @@ export type DiscoveredTestNode = DiscoveredTestCommon & {
export type DiscoveredTestPayload = {
cwd: string;
tests?: DiscoveredTestNode;
status: 'success' | 'error';
status: 'success' | 'error' | 'error-empty';
error?: string[];
};

Expand Down
21 changes: 20 additions & 1 deletion src/client/testing/testController/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,18 @@ export function populateTestTree(
resultResolver.runIdToVSid.set(child.runID, child.id_);
resultResolver.vsIdToRunId.set(child.id_, child.runID);
} else {
let node = testController.items.get(child.path);
// testRoot.children.get(child.path);
let node = testRoot!.children.get(child.path);
if (node !== undefined) {
console.log('WE MADE IT');
}
if (child.path.includes('.py')) {
console.log('IS A FILE DELETE CHIDLREN');
// delete the given file node from the children of its parent
testRoot!.children.delete(child.path);
// mark the node as undefined so a new node must be created below
node = undefined;
}

if (!node) {
node = testController.createTestItem(child.id_, child.name, Uri.file(child.path));
Expand Down Expand Up @@ -326,6 +337,14 @@ export function createDiscoveryErrorPayload(
};
}

export function createEmptyFileDiscoveryPayload(cwd: string, fileUri: Uri): DiscoveredTestPayload {
return {
cwd,
status: 'error-empty',
error: [fileUri.fsPath],
};
}

export function createEOTPayload(executionBool: boolean): EOTTestPayload {
return {
commandType: executionBool ? 'execution' : 'discovery',
Expand Down
10 changes: 10 additions & 0 deletions src/client/testing/testController/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,11 @@ export class PythonTestController implements ITestController, IExtensionSingleAc
}

private async refreshTestDataInternal(uri?: Resource): Promise<void> {
// the refresh ends up here (after being called on file save), the uri is of the file
// this method is confusing, it contains old and new code from before and after the rewrite, please make
// sure to focus on the rewrite as this will be the code we will keep moving forward
this.refreshingStartedEvent.fire();
// refresh button = uri is undefined
if (uri) {
const settings = this.configSettings.getSettings(uri);
const workspace = this.workspaceService.getWorkspaceFolder(uri);
Expand All @@ -268,14 +272,17 @@ export class PythonTestController implements ITestController, IExtensionSingleAc
// ** experiment to roll out NEW test discovery mechanism
if (settings.testing.pytestEnabled) {
if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) {
// this here is where the rewrite is enabled
traceInfo(`Running discovery for pytest using the new test adapter.`);
if (workspace && workspace.uri) {
const testAdapter = this.testAdapters.get(workspace.uri);
if (testAdapter) {
// this calls discovery, it need to be updated to include the file uri
testAdapter.discoverTests(
this.testController,
this.refreshCancellation.token,
this.pythonExecFactory,
uri,
);
} else {
traceError('Unable to find test adapter for workspace.');
Expand Down Expand Up @@ -317,6 +324,7 @@ export class PythonTestController implements ITestController, IExtensionSingleAc
// This handles the case where user removes test settings. Which should remove the
// tests for that particular case from the tree view
if (workspace) {
console.log('no longer deleting workspace', workspace);
const toDelete: string[] = [];
this.testController.items.forEach((i: TestItem) => {
const w = this.workspaceService.getWorkspaceFolder(i.uri);
Expand Down Expand Up @@ -549,11 +557,13 @@ export class PythonTestController implements ITestController, IExtensionSingleAc
}

private watchForTestContentChangeOnSave(): void {
// When a file is saved, it will trigger this action
this.disposables.push(
onDidSaveTextDocument(async (doc: TextDocument) => {
if (doc.fileName.endsWith('.py')) {
traceVerbose(`Testing: Trigger refresh after saving ${doc.uri.fsPath}`);
this.sendTriggerTelemetry('watching');
// it will call refresh and add the doc.uri (the file uri being saved)
this.refreshData.trigger(doc.uri, false);
}
}),
Expand Down
57 changes: 53 additions & 4 deletions src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
MESSAGE_ON_TESTING_OUTPUT_MOVE,
createDiscoveryErrorPayload,
createEOTPayload,
createEmptyFileDiscoveryPayload,
createTestingDeferred,
fixLogLinesNoTrailing,
} from '../common/utils';
Expand All @@ -39,7 +40,12 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
private readonly envVarsService?: IEnvironmentVariablesProvider,
) {}

async discoverTests(uri: Uri, executionFactory?: IPythonExecutionFactory): Promise<DiscoveredTestPayload> {
async discoverTests(
uri: Uri,
executionFactory?: IPythonExecutionFactory,
fileUri?: Uri,
): Promise<DiscoveredTestPayload> {
// after the workspaceTesetAdapter we jump here
const uuid = this.testServer.createUUID(uri.fsPath);
const deferredTillEOT: Deferred<void> = createDeferred<void>();
const dataReceivedDisposable = this.testServer.onDiscoveryDataReceived(async (e: DataReceivedEvent) => {
Expand All @@ -51,7 +57,8 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
dataReceivedDisposable.dispose();
};
try {
await this.runPytestDiscovery(uri, uuid, executionFactory);
// then we go here
await this.runPytestDiscovery(uri, uuid, executionFactory, fileUri);
} finally {
await deferredTillEOT.promise;
traceVerbose(`deferredTill EOT resolved for ${uri.fsPath}`);
Expand All @@ -62,11 +69,25 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
return discoveryPayload;
}

async runPytestDiscovery(uri: Uri, uuid: string, executionFactory?: IPythonExecutionFactory): Promise<void> {
async runPytestDiscovery(
uri: Uri,
uuid: string,
executionFactory?: IPythonExecutionFactory,
fileUri?: Uri,
): Promise<void> {
// this is where we really call the run discovery
const relativePathToPytest = 'pythonFiles';
const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest);
const settings = this.configSettings.getSettings(uri);
const { pytestArgs } = settings.testing;
/*
here we should be able to retrieve another setting which is the new setting we want to create.
I would name it something like `python.testing.DiscoveryOnOnlySavedFile`, I would need to talk with my team about an
official name so just use a placeholder for now. You can follow how other settings work (such as pytest args),
to get an idea on how to create a new setting and access it here in the code
*/
let { pytestArgs } = settings.testing;
const cwd = settings.testing.cwd && settings.testing.cwd.length > 0 ? settings.testing.cwd : uri.fsPath;

// get and edit env vars
Expand Down Expand Up @@ -98,6 +119,20 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
};
const execService = await executionFactory?.createActivatedEnvironment(creationOptions);
// delete UUID following entire discovery finishing.
/*
here we are putting together all the args for the pytest discovery.
THIS SECTION I AM OPEN TO SUGGESTIONS, i am not sure if my suggested flow below is the best so please let me know if you have a better idea.
1. check to see if -k is already being used
2. if it isn't then add `-k uriOfSavedFile` to the args
*/
if (fileUri !== undefined) {
// filter out arg "." if it exits
const filteredPytestArgs = pytestArgs.filter((arg) => arg !== '.');
filteredPytestArgs.push(fileUri.fsPath);
pytestArgs = filteredPytestArgs;
}
const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs);
traceVerbose(`Running pytest discovery with command: ${execArgs.join(' ')} for workspace ${uri.fsPath}.`);

Expand Down Expand Up @@ -143,6 +178,20 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
data: JSON.stringify(createEOTPayload(true)),
});
}

if (code === 5 && fileUri !== undefined) {
// if no tests are found, then we need to send the error payload.
this.testServer.triggerDiscoveryDataReceivedEvent({
uuid,
data: JSON.stringify(createEmptyFileDiscoveryPayload(cwd, fileUri)),
});
// then send a EOT payload
this.testServer.triggerDiscoveryDataReceivedEvent({
uuid,
data: JSON.stringify(createEOTPayload(true)),
});
}

// deferredTillEOT is resolved when all data sent on stdout and stderr is received, close event is only called when this occurs
// due to the sync reading of the output.
deferredTillExecClose?.resolve();
Expand Down
7 changes: 6 additions & 1 deletion src/client/testing/testController/workspaceTestAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,10 @@ export class WorkspaceTestAdapter {
testController: TestController,
token?: CancellationToken,
executionFactory?: IPythonExecutionFactory,
fileUri?: Uri,
): Promise<void> {
// Here is where discover tests goes when called by the controller
// this then determines which adapter to use (either the run or discovery one)
sendTelemetryEvent(EventName.UNITTEST_DISCOVERING, undefined, { tool: this.testProvider });

// Discovery is expensive. If it is already running, use the existing promise.
Expand All @@ -128,8 +131,10 @@ export class WorkspaceTestAdapter {
try {
// ** execution factory only defined for new rewrite way
if (executionFactory !== undefined) {
await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory);
// goes here
await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory, fileUri);
} else {
// ignore this, it is the old way
await this.discoveryAdapter.discoverTests(this.workspaceUri);
}
deferred.resolve();
Expand Down

0 comments on commit 7fe16ee

Please sign in to comment.