Discovery cancellation #24713
Discovery cancellation #24713
Conversation
There was a problem hiding this comment.
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- src/client/testing/testController/workspaceTestAdapter.ts: Evaluated as low risk
- src/client/testing/testController/pytest/pytestExecutionAdapter.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts:130
- Redundant cancellation handling. The
token?.onCancellationRequestedcallback is defined twice, once forprocand once forresultProc. This can be simplified to avoid confusion.
token?.onCancellationRequested(() => {
Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more
| token?.onCancellationRequested(() => { | ||
| traceInfo(`Test discovery cancelled.`); | ||
| cSource.cancel(); | ||
| deferredReturn.resolve({ cwd: uri.fsPath, status: 'success' }); |
There was a problem hiding this comment.
The status message should indicate that the operation was canceled, not 'success'. Consider using a status like 'canceled'.
| deferredReturn.resolve({ cwd: uri.fsPath, status: 'success' }); | |
| deferredReturn.resolve({ cwd: uri.fsPath, status: 'canceled' }); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
so the actual return value isn't really used at all in production code- currently it is just awaited on the workspaceTestAdapter. Should I just made it an empty promise and remove this whole "status" concept @karthiknadig
There was a problem hiding this comment.
That might make it more readable. So, make it a empty promise, now that old code is all removed.
|
This PR originates from a fork. If the changes appear safe, you can trigger the pipeline by commenting |
fixes #24602