Use native VS Code test UI on canary by dbartol · Pull Request #2306 · github/vscode-codeql

dbartol · 2023-04-12T20:09:20Z

I decided to take a look at what it would take to move away from the third-party Test Explorer extension for CodeQL tests, and it turned out to be easier than I expected. This PR moves CodeQL testing to use the native VS Code test UI when the canary flag is set. If everything seems to work well after a bit of internal dogfooding, we can remove the old code entirely and enable this for everyone.

Code changes

To access the new VS Code testing API, I needed to update our @types/vscode dependency. This exposed a name collision in some unrelated quick pick code, which I fixed in a separate commit.
I was able to reuse much of the existing test-related code. In particular, the test discovery code is entirely unchanged.
- The code to run a set of tests, including unloading databases beforehand and reloading them afterward, has been factored out into a TestRunner service, used by both the legacy and the new test managers.
- The implementation of the two custom test commands (acceptOutput and showOutputDiffs) has been moved into a new TestManagerBase class, from which both the old and new test manager classes inhierit.
The CLI server has been updated to use BaseLogger instead of Logger, since it doesn't need the show() function.
I tweaked the code that runs an async CLI command to await the child process even on failure, to avoid a runtime error about the promise rejection being unhandled.
I added the actual property to TestCompleted, since it was already being set by the CLI.
I implemented the new support in the new TestManager service. The model for interacting with the native VS Code test UI is quite a bit simpler than the third-party extension.
- We create a TestController object and update its list of TestItems as we discover tests.
- When we run the tests, the controller passes us a TestRunRequest that tells us what to run, and we call the shared TestRunner service to handle the CLI invocation and communication.
- Whenever a test completes, we notify the TestRun object. Test results can include TestMessage objects, which can have file locations, and can contain diffs between actual and expected text. For compilation failures, we emit the compilation errors as TestMessages. For actual/expected mismatch failure, we create a TestMessage containing the result diff.
- All human-readable output from the CLI invocation is logged via TestRun.logOutput(), which sends it to the "Test Results" tab.

Test changes

Moved two of the existing test adapter tests into test-runner.test.ts, since they really only test the functionality that is now factored out into the TestRunner class.
Added a case to test-adapter.test.ts to test the new test controller similar to how we test the legacy one.
Moved some shared code into a test-runner-helpers.ts file.

aeisenberg

Thanks for taking this on.

aeisenberg · 2023-04-12T20:46:09Z

+  ctx.subscriptions.push(testRunner);
+
+  let testManager: TestManagerBase | undefined = undefined;
+  if (isCanary()) {


As much as possible, we try to dis/enable all canary features without a restart. It looks like this is not following the pattern. I don't know how easy it would be to do, though.

Hmm. There's no way to "unregister" a TestController AFAICT, but maybe just deleting all of its TestItems would be equivalent. I'll have to see if there's a way to unregister the legacy test adapter, but it might not be worth the effort.

OK, there is a way to unregister the legacy test stuff. However, handling of the commands we expose is trickier. We share the same command IDs between the two implementations, but they route to slightly different implementations depending on which test UI we're using.

Given how easy the transition to the new UI was, and how annoying our dependency on that third-party extension is, I suspect we'll move this out of canary pretty soon anyway.

If it's going to take significant time and still be imperfect, I don't think it's worth taking on for a transitional feature.

aeisenberg · 2023-04-12T21:13:39Z

+
+      if (!(await pathExists(expectedPath))) {
+        void showAndLogWarningMessage(
+          `'${basename(expectedPath)}' does not exist. Creating an empty file.`,


I know you are just moving this around, but it seems odd that we are popping up a warning box here. Maybe a bit annoying. I think better to just log it in the outpuut view without a popup and create the file.

I've just removed the error message altogether. I don't think it's interesting to log.

aeisenberg · 2023-04-12T21:17:49Z

+ */
+async function tryReadFileContents(
+  path: string,
+  testMessages: TestMessage[],


I'd slightly prefer changing this function so that it actually throws the error message and the call site is wrapped in a try-catch that would add to the testMessages array. This is more stylistic, so what you have now is also fine.

I only did it this way to avoid duplicating the code that formats and adds the test message. For a non-exported function, I think it's OK as-is.

charisk · 2023-04-13T09:20:15Z

Would it be possible to extract upgrading @types/vscode (and resolving naming conflicts) into a separate PR please? That would make it easier to review and would give us some nicer git history around changes to the db panel code.

aeisenberg · 2023-04-13T20:15:58Z

In a separate PR, we should probably be moving all of the test-*.ts files into a test directory. We are moving towards that for all of our other ts modules that are similar to each other.

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

dbartol · 2023-04-13T21:11:03Z

Would it be possible to extract upgrading @types/vscode (and resolving naming conflicts) into a separate PR please? That would make it easier to review and would give us some nicer git history around changes to the db panel code.

#2323

aeisenberg · 2023-04-13T22:34:45Z

    private readonly testController: TestController = tests.createTestController(
      "codeql",
-      "Fancy CodeQL Tests",
+      "CodeQL Tests",


aeisenberg · 2023-04-13T22:52:44Z

Just deleted two comments that were meant for #2307

koesie10 · 2023-04-13T08:13:45Z

+      try {
+        await childPromise;
+      } catch (_e) {
+        // We need to await this to avoid an unhandled rejection, but we want to propagate the
+        // original exception.
+      }


Would childPromise.catch(() => void 0); have the same effect? We wouldn't need to wait on the promise but still no unhandled rejection will be shown.

koesie10 · 2023-04-14T14:20:22Z

+            ].join("\n")
+          : [
+              "",
+              `${event.failureStage?.toLowerCase()} error: ${event.test}`,


This will print undefined error if the failureStage is empty. Should we printing an empty string or unknown instead?

koesie10 · 2023-04-14T14:26:58Z

+    if (node.uri === undefined || node.uri.scheme !== "file") {
+      throw new Error("Selected test is not a CodeQL test.");
+    }
+    return node.uri!.fsPath;


TypeScript should recognize that node.uri is now not undefined:

Suggested change

return node.uri!.fsPath;

return node.uri.fsPath;

koesie10 · 2023-04-14T14:29:44Z

+      const tests: string[] = [];
+      testsToRun.forEach((t) => {
+        testRun.enqueued(t);
+        tests.push(t.uri!.fsPath);


Can we do something different here, like returning if t.uri === undefined?

We actually already had the path we need in the key of the map entry, so I used that instead.

…e-codeql into dbartol/new-test-ui

dbartol · 2023-04-20T13:59:10Z

Related: #1099

Dave Bartolomeo added 3 commits April 12, 2023 15:49

Avoid identifier collision with new VS Code version

e1dae0b

Use native VS Code test UI in canary

4804220

Merge remote-tracking branch 'origin/main' into dbartol/new-test-ui

7ea7d6c

dbartol added Complexity: Medium Requires a moderate level of detail in design or review. Update dependencies Dependabot update PRs tech-debt labels Apr 12, 2023

github-advanced-security AI found potential problems Apr 12, 2023

View reviewed changes

Comment thread extensions/ql-vscode/package.json Fixed

Dave Bartolomeo added 2 commits April 12, 2023 16:45

Add missing mock method after @types/vscode upgrade

e986b07

Fix test adapter tests

397b585

aeisenberg reviewed Apr 12, 2023

View reviewed changes

Dave Bartolomeo and others added 3 commits April 13, 2023 17:00

Test tests

cb93c84

Update extensions/ql-vscode/src/test-manager.ts

eca8cce

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

Merge branch 'main' into dbartol/new-test-ui

c963cf6

dbartol marked this pull request as ready for review April 13, 2023 21:15

dbartol requested review from a team as code owners April 13, 2023 21:15

Dave Bartolomeo added 4 commits April 13, 2023 17:27

Add separate command for "Accept Output" on test item context menu

8f29e1c

Merge branch 'dbartol/update-vscode-types' into dbartol/new-test-ui

c9507df

Reconsider the fanciness of CodeQL tests

da3f482

Merge branch 'main' into dbartol/new-test-ui

8f5f43f

aeisenberg reviewed Apr 13, 2023

View reviewed changes

koesie10 reviewed Apr 14, 2023

View reviewed changes

Dave Bartolomeo added 2 commits April 14, 2023 15:24

Fix PR feedback

8c2c25e

Merge branch 'dbartol/new-test-ui' of https://github.com/github/vscod…

a52391b

…e-codeql into dbartol/new-test-ui

aeisenberg approved these changes Apr 15, 2023

View reviewed changes

dbartol merged commit 70b4aac into main Apr 17, 2023

dbartol deleted the dbartol/new-test-ui branch April 17, 2023 17:49

dbartol mentioned this pull request Apr 20, 2023

Migrate to new vscode testing APIs #1099

Closed

Conversation

dbartol commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code changes

Test changes

Uh oh!

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charisk commented Apr 13, 2023

Uh oh!

aeisenberg commented Apr 13, 2023

Uh oh!

dbartol commented Apr 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Apr 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbartol commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dbartol commented Apr 12, 2023 •

edited

Loading