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.

Sign up for GitHub

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

per file discovery on save #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

per file discovery on save #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 14 additions & 1 deletion src/client/testing/testController/common/resultResolver.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link

Choose a reason for hiding this comment

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

Thanks for putting it together. I believe you also need to push "-k" at the beginning unless I misunderstood how this part works. But I think this only affects running tests, i.e. after collecting, so it probably doesn't affect performance.
I have also seen that there are other options:

  --ignore=path         ignore path during collection (multi-allowed).
  --ignore-glob=path 
  --override-ini python_files (args):  glob-style file patterns for Python test module discovery

Maybe python_files is the one to use, but I also dont know if it will work since it may not support full file paths. 😞

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
View file
Open in desktop
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
Loading