Implement QL Test support #173

dbartol · 2019-11-20T21:45:27Z

The PR contains the initial implementing of QL Test support in CodeQL for Visual Studio Code (https://github.com/github/pe-security-codeql/issues/28). Because QL Test support isn't quite ready in the CLI yet, this PR uses odasa to run the tests for now. As CLI support comes online, it should be straightforward to swap out the implementation to use the CLI. This is now implemented using codeql tests run.

The treeview UI for the tests is implemented via the hbenl.vscode-test-explorer extension. This extension is open source, and appears to be actively maintained. It's used by a couple dozen existing extensions for tests for various languages. The extension doesn't really do anything on its own, so taking it as a dependency isn't introducing any unwanted UI clutter. Note that I did have to remove the --disable-extensions argument from launch.json, because otherwise the test explorer extension gets disabled, preventing our own extension from loading.

The UI will display a root node for each QL pack that contains tests, with the actual test directories and files as descendants of that root node. We consider only those QL packs in the workspace; QL packs on the default CodeQL search path are ignored. We use codeql resolve qlpacks to find the packs, and then watch all qlpack.yml files in the workspace for changes in order to refresh the pack discovery when necessary. Ideally, we'd have the CLI return a set of path patterns to watch, but for now the current implementation works fine.

To discover the tests within a given pack, ~~we walk the pack's directory tree manually for now, until the relevant CLI command is available~~we use codeql resolve tests. Because we do not yet have a mechanism in qlpack.yml to specify whether or not the pack contains tests, we assume that any pack whose name ends with "-tests" to contain nothing but tests, and any other pack to contain no tests. This is sufficient for the tests in the QL repo. As with QL pack discovery, we watch the file system for changes in .ql and .qlref files in order to refresh the tree of tests if anything changes.

To actually run the tests, we just invoke codeql test run with the appropriate arguments.

The test-ui.ts file implements a couple of additional commands for the context menu of the test treeview. You can accept the output of a failing test (copying the .actual file to the .expected file), and you can bring up a diff view of the .expected and .actual files).

This PR includes a couple of related utility classes. UIService makes it a little easier to implement a service that handles VS Code commands. Discovery is a base class that handles most of the work that is shared between the different kinds of discovery that we do, like avoiding running multiple discovery operations simultaneously if we get a storm of file change notifications.

The PR contains the initial implementing of QL Test support in CodeQL for Visual Studio Code. Because QL Test support isn't quite ready in the CLI yet, this PR uses `odasa` to run the tests for now. As CLI support comes online, it should be straightforward to swap out the implementation to use the CLI. The treeview UI for the tests is implemented via the `hbenl.vscode-test-explorer` extension. This extension is open source, and appears to be actively maintained. It's used by a couple dozen existing extensions for tests for various languages. The extension doesn't really do anything on its own, so taking it as a dependency isn't introducing any unwanted UI clutter. Note that I did have to remove the `--disable-extensions` argument from `launch.json`, because otherwise the test explorer extension gets disabled, preventing our own extension from loading. The UI will display a root node for each QL pack that contains tests, with the actual test directories and files as descendants of that root node. We consider only those QL packs in the workspace; QL packs on the default CodeQL search path are ignored. We use `codeql resolve qlpacks` to find the packs, and then watch all `qlpack.yml` files in the workspace for changes in order to refresh the pack discovery when necessary. Ideally, we'd have the CLI return a set of path patterns to watch, but for now the current implementation works fine. To discover the tests within a given pack, we walk the pack's directory tree manually for now, until the relevant CLI command is available. Because we do not yet have a mechanism in `qlpack.yml` to specify whether or not the pack contains tests, we assume that any pack whose name ends with "-tests" to contain nothing but tests, and any other pack to contain no tests. This is sufficient for the tests in the QL repo. As with QL pack discovery, we watch the file system for changes in `.ql` and `.qlref` files in order to refresh the tree of tests if anything changes. To actually run the tests, we just invoke `odasa qltest` with the appropriate arguments. This code is pretty much a straight copy-and-paste from the repo where I've had a private version of QL Test support for several months. Once we can run tests via the CLI, this will all be deleted. The `test-ui.ts` file implements a couple of additional commands for the context menu of the test treeview. You can accept the output of a failing test (copying the `.actual` file to the `.expected` file), and you can bring up a diff view of the `.expected` and `.actual` files). This PR includes a couple of related utility classes. `UIService` makes it a little easier to implement a service that handles VS Code commands. `Discovery` is a base class that handles most of the work that is shared between the different kinds of discovery that we do, like avoiding running multiple discovery operations simultaneously if we get a storm of file change notifications.

lgtm-com · 2019-11-20T21:49:32Z

This pull request introduces 2 alerts when merging 444aca3 into a559404 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

dbartol · 2019-11-21T00:21:07Z

I've added a much better error message when trying to run tests without Semmle Core configured. Since we may be waiting a while for CLI test support, I think we should merge this PR now rather than let it rot while we wait.

jcreedcmu · 2019-11-21T14:40:34Z

.vscode/launch.json

@@ -8,8 +8,7 @@
      "request": "launch",
      "runtimeExecutable": "${execPath}",
      "args": [
-        "--extensionDevelopmentPath=${workspaceRoot}/dist/vscode-codeql",
-        "--disable-extensions"


I don't quite understand the reason for removing --disable-extensions? I thought we had it in here to ensure that integration test failures couldn't be because some other extension was confusing things.

Our extension now depends on the "VS Code Test Explorer" extension. If we launch VS Code with --disable-extensions, it disables the "VS Code Test Explorer" extension, and therefore refuses to load our own extension due to the disabled dependency.

adityasharad

First pass, still need to look into the watching/discovery code more carefully.

adityasharad · 2019-11-21T21:20:26Z

extensions/ql-vscode/package.json

  "activationEvents": [
    "onLanguage:ql",
    "onView:codeQLDatabases",
    "onView:codeQLQueryHistory",
+    "onView:test-explorer",


Do we need this? If you have the test explorer open but no CodeQL workspace then I'm not sure we need to activate yet.

I think we need it for the same reason we need the other onView: events: If I open a workspace that contains CodeQL tests, but have not yet opened a .ql file, I should still be able to browse and run my tests.

extensions/ql-vscode/package.json

adityasharad · 2019-11-21T21:21:15Z

extensions/ql-vscode/package.json

+        },
+        "codeQL.tests.numberOfThreads": {
+          "scope": "window",
+          "type": "integer",


Default to 1?

Fixed to be 1, allowed range [1, 1024], just like the number of threads for queries.

adityasharad · 2019-11-21T21:48:56Z

extensions/ql-vscode/src/qltest-discovery.ts

+import { DisposableObject } from 'semmle-vscode-utils';
+
+/**
+ * A node in the tree of tests. This will be either a `QLTestDirector` or a `QLTestFile`.


Suggested change

* A node in the tree of tests. This will be either a `QLTestDirector` or a `QLTestFile`.

* A node in the tree of tests. This will be either a `QLTestDirectory` or a `QLTestFile`.

adityasharad · 2019-11-21T21:57:52Z

extensions/ql-vscode/src/qltest-discovery.ts

+    switch (path.extname(fullPath).toLowerCase()) {
+      case '.ql':
+      case '.qlref': {
+        if (!path.basename(fullPath).startsWith('__')) {


What is __ for?

extensions/ql-vscode/package.json

jcreedcmu · 2020-02-18T16:41:46Z

extensions/ql-vscode/src/cli.ts

-export interface ResolvedQLPacks {
-  [index: string]: string[];
-}
-


Seems to be used in src/qlpack-discovery.ts, actually

Oops. Fixed.

jcreedcmu

General comment: If I have run quick query previously on a workspace, a directory 'Quick Query' appears in the test panel with no tests in it. I don't know if you expected this or not --- it is the case that the directory has a qlpacks.yml file in it, so it makes sense that it shows up during qlpack discovery. Maybe we should cull directories that have no tests?

extensions/ql-vscode/src/queryserver-client.ts

extensions/ql-vscode/src/qltest-discovery.ts

This reverts commit 39805bc.

dbartol added 5 commits November 20, 2019 14:55

Fix LGTM alerts

ddf42d8

Attempt to fix pure test failure

85f7ff1

Activate when Test Explorer view is opened

be72e9b

Better error message when odasa is not configured properly

e37807c

odasa -> Semmle Core

cbe3c05

dbartol marked this pull request as ready for review November 21, 2019 00:21

jcreedcmu reviewed Nov 21, 2019

View reviewed changes

dbartol added 3 commits November 21, 2019 14:13

Fix file-watching bug in QLTest discovery

7d325e3

Merge remote-tracking branch 'upstream/master' into dbartol/QLTest

9e6100f

Merge from master

2f9a314

adityasharad reviewed Dec 5, 2019

View reviewed changes

dbartol added 8 commits December 16, 2019 15:41

Merge from master

55d1a4a

Make QLTest use codeql test run instead of odasa qltest

de2a6cc

Merge remote-tracking branch 'upstream/master' into dbartol/QLTest

207743e

Implement cancellation of test run

0947a35

Merge remote-tracking branch 'upstream/master' into dbartol/QLTest

edc1f1c

Merge from master

90a9753

Fix typo

1b7d0da

Remove unused interface

afcc05f

dbartol requested review from jcreedcmu and adityasharad February 15, 2020 01:07

dbartol added the enhancement New feature or request label Feb 15, 2020

jcreedcmu reviewed Feb 18, 2020

View reviewed changes

dbartol changed the title ~~Implement QL Test support (using odasa for now)~~ Implement QL Test support Feb 18, 2020

Fix references to renamed type

f2c525b

jcreedcmu suggested changes Feb 25, 2020

View reviewed changes

extensions/ql-vscode/src/queryserver-client.ts Show resolved Hide resolved

extensions/ql-vscode/src/qltest-discovery.ts Show resolved Hide resolved

Merge from master

984ba73

dbartol added 3 commits March 3, 2020 15:33

Sort tests by name

370444c

Remove unnecessary? import hack

39805bc

Revert "Remove unnecessary? import hack"

7fc501f

This reverts commit 39805bc.

jcreedcmu mentioned this pull request Mar 9, 2020

Fix dependency of queryserver client on compiled code #268

Closed

jcreedcmu approved these changes Mar 9, 2020

View reviewed changes

jcreedcmu merged commit 9377279 into github:master Mar 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement QL Test support #173

Implement QL Test support #173

dbartol commented Nov 20, 2019 •

edited

Loading

lgtm-com bot commented Nov 20, 2019

dbartol commented Nov 21, 2019

jcreedcmu Nov 21, 2019

dbartol Nov 21, 2019

adityasharad left a comment

adityasharad Nov 21, 2019

dbartol Feb 15, 2020

adityasharad Nov 21, 2019

dbartol Feb 15, 2020

adityasharad Nov 21, 2019

dbartol Feb 15, 2020

adityasharad Nov 21, 2019

jcreedcmu Feb 18, 2020

dbartol Feb 18, 2020

jcreedcmu left a comment

	* A node in the tree of tests. This will be either a `QLTestDirector` or a `QLTestFile`.
	* A node in the tree of tests. This will be either a `QLTestDirectory` or a `QLTestFile`.

Implement QL Test support #173

Implement QL Test support #173

Conversation

dbartol commented Nov 20, 2019 • edited Loading

lgtm-com bot commented Nov 20, 2019

dbartol commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityasharad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcreedcmu left a comment

Choose a reason for hiding this comment

dbartol commented Nov 20, 2019 •

edited

Loading