Restructure the tree in the Test Explorer View

With this change we display the tree based on the file system not based
on ql-packs. We also merge test folders whose only child is another
test folder.

Resolves #595

Author: aeisenberg

extensions/ql-vscode/src/cli.ts

@@ -470,7 +470,11 @@ export class CodeQLCliServer implements Disposable {
     const subcommandArgs = [
       testPath
     ];
-    return await this.runJsonCodeQlCliCommand<ResolvedTests>(['resolve', 'tests'], subcommandArgs, 'Resolving tests');
+    return await this.runJsonCodeQlCliCommand<ResolvedTests>(
+      ['resolve', 'tests', '--strict-test-discovery'],
+      subcommandArgs,
+      'Resolving tests'
+    );
   }
 
   /**

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

@@ -1,7 +1,7 @@
 import * as path from 'path';
-import { QLPackDiscovery, QLPack } from './qlpack-discovery';
+import { QLPackDiscovery } from './qlpack-discovery';
 import { Discovery } from './discovery';
-import { EventEmitter, Event, Uri, RelativePattern, WorkspaceFolder, env, workspace } from 'vscode';
+import { EventEmitter, Event, Uri, RelativePattern, WorkspaceFolder, env } from 'vscode';
 import { MultiFileSystemWatcher } from './vscode-utils/multi-file-system-watcher';
 import { CodeQLCliServer } from './cli';
 
@@ -29,9 +29,8 @@ export abstract class QLTestNode {
  * A directory containing one or more QL tests or other test directories.
  */
 export class QLTestDirectory extends QLTestNode {
-  private _children: QLTestNode[] = [];
 
-  constructor(_path: string, _name: string) {
+  constructor(_path: string, _name: string, private _children: QLTestNode[] = []) {
     super(_path, _name);
   }
 
@@ -55,10 +54,23 @@ export class QLTestDirectory extends QLTestNode {
   }
 
   public finish(): void {
+    // remove empty directories
+    this._children.filter(child =>
+      child instanceof QLTestFile || child.children.length > 0
+    );
     this._children.sort((a, b) => a.name.localeCompare(b.name, env.language));
-    for (const child of this._children) {
+    this._children.forEach((child, i) => {
       child.finish();
-    }
+      if (child.children?.length === 1 && child.children[0] instanceof QLTestDirectory) {
+        // collapse children
+        const replacement = new QLTestDirectory(
+          child.children[0].path,
+          child.name + ' / ' + child.children[0].name,
+          Array.from(child.children[0].children)
+        );
+        this._children[i] = replacement;
+      }
+    });
   }
 
   private createChildDirectory(name: string): QLTestDirectory {
@@ -96,14 +108,15 @@ export class QLTestFile extends QLTestNode {
  */
 interface QLTestDiscoveryResults {
   /**
-   * The root test directory for each QL pack that contains tests.
+   * A directory that contains one or more QL Tests, or other QLTestDirectories.
    */
-  testDirectories: QLTestDirectory[];
+  testDirectory: QLTestDirectory | undefined;
+
   /**
-   * The list of file system paths to watch. If any of these paths changes, the discovery results
-   * may be out of date.
+   * The file system path to a directory to watch. If any ql or qlref file changes in
+   * this directory, then this signifies a change in tests.
    */
-  watchPaths: string[];
+  watchPath: string;
 }
 
 /**
@@ -112,7 +125,7 @@ interface QLTestDiscoveryResults {
 export class QLTestDiscovery extends Discovery<QLTestDiscoveryResults> {
   private readonly _onDidChangeTests = this.push(new EventEmitter<void>());
   private readonly watcher: MultiFileSystemWatcher = this.push(new MultiFileSystemWatcher());
-  private _testDirectories: QLTestDirectory[] = [];
+  private _testDirectory: QLTestDirectory | undefined;
 
   constructor(
     private readonly qlPackDiscovery: QLPackDiscovery,
@@ -128,12 +141,17 @@ export class QLTestDiscovery extends Discovery<QLTestDiscoveryResults> {
   /**
    * Event to be fired when the set of discovered tests may have changed.
    */
-  public get onDidChangeTests(): Event<void> { return this._onDidChangeTests.event; }
+  public get onDidChangeTests(): Event<void> {
+    return this._onDidChangeTests.event;
+  }
 
   /**
-   * The root test directory for each QL pack that contains tests.
+   * The root directory. There is at least one test in this directory, or
+   * in a subdirectory of this.
    */
-  public get testDirectories(): QLTestDirectory[] { return this._testDirectories; }
+  public get testDirectory(): QLTestDirectory | undefined {
+    return this._testDirectory;
+  }
 
   private handleDidChangeQLPacks(): void {
     this.refresh();
@@ -144,72 +162,49 @@ export class QLTestDiscovery extends Discovery<QLTestDiscoveryResults> {
       this.refresh();
     }
   }
-
+  static cnt = 0;
   protected async discover(): Promise<QLTestDiscoveryResults> {
-    const testDirectories: QLTestDirectory[] = [];
-    const watchPaths: string[] = [];
-    const qlPacks = this.qlPackDiscovery.qlPacks;
-    for (const qlPack of qlPacks) {
-      //HACK: Assume that only QL packs whose name ends with '-tests' contain tests.
-      if (this.isRelevantQlPack(qlPack)) {
-        watchPaths.push(qlPack.uri.fsPath);
-        const testPackage = await this.discoverTests(qlPack.uri.fsPath, qlPack.name);
-        if (testPackage !== undefined) {
-          testDirectories.push(testPackage);
-        }
-      }
-    }
-
-    return { testDirectories, watchPaths };
+    const timer = 'testDirectory-' + this.workspaceFolder.uri.fsPath + '-' + QLTestDiscovery.cnt++;
+    console.time(timer);
+    const testDirectory = await this.discoverTests();
+    console.timeEnd(timer);
+    return {
+      testDirectory,
+      watchPath: this.workspaceFolder.uri.fsPath
+    };
   }
 
   protected update(results: QLTestDiscoveryResults): void {
-    this._testDirectories = results.testDirectories;
+    this._testDirectory = results.testDirectory;
 
     // Watch for changes to any `.ql` or `.qlref` file in any of the QL packs that contain tests.
     this.watcher.clear();
-    results.watchPaths.forEach(watchPath => {
-      this.watcher.addWatch(new RelativePattern(watchPath, '**/*.{ql,qlref}'));
-    });
+    this.watcher.addWatch(new RelativePattern(results.watchPath, '**/*.{ql,qlref}'));
     this._onDidChangeTests.fire();
   }
 
-  /**
-   * Only include qlpacks suffixed with '-tests' that are contained
-   * within the provided workspace folder.
-   */
-  private isRelevantQlPack(qlPack: QLPack): boolean {
-    return qlPack.name.endsWith('-tests')
-      && workspace.getWorkspaceFolder(qlPack.uri)?.index === this.workspaceFolder.index;
-  }
-
   /**
    * Discover all QL tests in the specified directory and its subdirectories.
-   * @param fullPath The full path of the test directory.
-   * @param name The display name to use for the returned `TestDirectory` object.
    * @returns A `QLTestDirectory` object describing the contents of the directory, or `undefined` if
    *   no tests were found.
    */
-  private async discoverTests(fullPath: string, name: string): Promise<QLTestDirectory | undefined> {
+  private async discoverTests(): Promise<QLTestDirectory> {
+    const fullPath = this.workspaceFolder.uri.fsPath;
+    const name = this.workspaceFolder.name;
     const resolvedTests = (await this.cliServer.resolveTests(fullPath))
       .filter((testPath) => !QLTestDiscovery.ignoreTestPath(testPath));
 
-    if (resolvedTests.length === 0) {
-      return undefined;
+    const rootDirectory = new QLTestDirectory(fullPath, name);
+    for (const testPath of resolvedTests) {
+      const relativePath = path.normalize(path.relative(fullPath, testPath));
+      const dirName = path.dirname(relativePath);
+      const parentDirectory = rootDirectory.createDirectory(dirName);
+      parentDirectory.addChild(new QLTestFile(testPath, path.basename(testPath)));
     }
-    else {
-      const rootDirectory = new QLTestDirectory(fullPath, name);
-      for (const testPath of resolvedTests) {
-        const relativePath = path.normalize(path.relative(fullPath, testPath));
-        const dirName = path.dirname(relativePath);
-        const parentDirectory = rootDirectory.createDirectory(dirName);
-        parentDirectory.addChild(new QLTestFile(testPath, path.basename(testPath)));
-      }
 
-      rootDirectory.finish();
+    rootDirectory.finish();
 
-      return rootDirectory;
-    }
+    return rootDirectory;
   }
 
   /**

extensions/ql-vscode/src/test-adapter.ts

@@ -99,6 +99,7 @@ export class QLTestAdapter extends DisposableObject implements TestAdapter {
 
     this.qlPackDiscovery = this.push(new QLPackDiscovery(workspaceFolder, cliServer));
     this.qlTestDiscovery = this.push(new QLTestDiscovery(this.qlPackDiscovery, workspaceFolder, cliServer));
+    // TODO: Don't run test discovery until pack discovery finishes
     this.qlPackDiscovery.refresh();
     this.qlTestDiscovery.refresh();
 
@@ -160,22 +161,22 @@ export class QLTestAdapter extends DisposableObject implements TestAdapter {
   private discoverTests(): void {
     this._tests.fire({ type: 'started' } as TestLoadStartedEvent);
 
-    const testDirectories = this.qlTestDiscovery.testDirectories;
-    const children = testDirectories.map(
-      testDirectory => QLTestAdapter.createTestSuiteInfo(testDirectory, testDirectory.name)
-    );
-    const testSuite: TestSuiteInfo = {
-      type: 'suite',
-      label: 'CodeQL',
-      id: '.',
-      children
-    };
-
+    const testDirectory = this.qlTestDiscovery.testDirectory;
+    let testSuite: TestSuiteInfo | undefined;
+    if (testDirectory?.children.length) {
+      const children = QLTestAdapter.createTestOrSuiteInfos(testDirectory.children);
+      testSuite = {
+        type: 'suite',
+        label: 'CodeQL',
+        id: testDirectory.path,
+        children
+      };
+    }
     this._tests.fire({
       type: 'finished',
-      suite: children.length > 0 ? testSuite : undefined
+      suite: testSuite
     } as TestLoadFinishedEvent);
-  }
+}
 
   public async run(tests: string[]): Promise<void> {
     if (this.runningTask !== undefined) {

extensions/ql-vscode/src/vscode-tests/minimal-workspace/qltest-discovery.test.ts

@@ -1,40 +1,61 @@
 import 'vscode-test';
 import 'mocha';
-import * as path from 'path';
-import { Uri, workspace } from 'vscode';
+import { Uri, WorkspaceFolder } from 'vscode';
 import { expect } from 'chai';
 
 import { QLTestDiscovery } from '../../qltest-discovery';
 
 describe('qltest-discovery', () => {
-  describe('isRelevantQlPack', () => {
-    it('should check if a qlpack is relevant', () => {
-      const qlTestDiscover: any = new QLTestDiscovery(
+  describe('discoverTests', () => {
+    it('should run discovery', async () => {
+      const baseUri = Uri.parse('file:/a/b');
+      const baseDir = baseUri.fsPath;
+      const cDir = Uri.parse('file:/a/b/c').fsPath;
+      const dFile = Uri.parse('file:/a/b/c/d.ql').fsPath;
+      const eFile = Uri.parse('file:/a/b/c/e.ql').fsPath;
+      const hDir = Uri.parse('file:/a/b/c/f/g/h').fsPath;
+      const iFile = Uri.parse('file:/a/b/c/f/g/h/i.ql').fsPath;
+      const qlTestDiscover = new QLTestDiscovery(
         { onDidChangeQLPacks: () => ({}) } as any,
-        { index: 0 } as any,
-        {} as any
+        {
+          uri: baseUri,
+          name: 'My tests'
+        } as unknown as WorkspaceFolder,
+        {
+          resolveTests() {
+            return [
+              Uri.parse('file:/a/b/c/d.ql').fsPath,
+              Uri.parse('file:/a/b/c/e.ql').fsPath,
+              Uri.parse('file:/a/b/c/f/g/h/i.ql').fsPath
+            ];
+          }
+        } as any
       );
 
-      const uri = workspace.workspaceFolders![0].uri;
-      expect(qlTestDiscover.isRelevantQlPack({
-        name: '-hucairz',
-        uri
-      })).to.be.false;
+      const result = await (qlTestDiscover as any).discover();
+      expect(result.watchPath).to.eq(baseDir);
+      expect(result.testDirectory.path).to.eq(baseDir);
+      expect(result.testDirectory.name).to.eq('My tests');
 
-      expect(qlTestDiscover.isRelevantQlPack({
-        name: '-tests',
-        uri: Uri.file('/a/b/')
-      })).to.be.false;
+      let children = result.testDirectory.children;
+      expect(children[0].path).to.eq(cDir);
+      expect(children[0].name).to.eq('c');
+      expect(children.length).to.eq(1);
 
-      expect(qlTestDiscover.isRelevantQlPack({
-        name: '-tests',
-        uri
-      })).to.be.true;
+      children = children[0].children;
+      expect(children[0].path).to.eq(dFile);
+      expect(children[0].name).to.eq('d.ql');
+      expect(children[1].path).to.eq(eFile);
+      expect(children[1].name).to.eq('e.ql');
 
-      expect(qlTestDiscover.isRelevantQlPack({
-        name: '-tests',
-        uri: Uri.file(path.join(uri.fsPath, 'other'))
-      })).to.be.true;
+      // A merged foler
+      expect(children[2].path).to.eq(hDir);
+      expect(children[2].name).to.eq('f / g / h');
+      expect(children.length).to.eq(3);
+
+      children = children[2].children;
+      expect(children[0].path).to.eq(iFile);
+      expect(children[0].name).to.eq('i.ql');
     });
   });
 });