Reregister testproj databases around test runs

To deal with the problem of CodeQL tests modifying open testproj databases,
this commit removes open databases from the extension prior to running tests,
and tries to open those databases again after tests finish running.

Author: cklin

extensions/ql-vscode/CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - Display CodeQL CLI version being downloaded during an upgrade. [#862](https://github.com/github/vscode-codeql/pull/862)
 - Display a helpful message and link to documentation when a query produces no results. [#866](https://github.com/github/vscode-codeql/pull/866)
+- Refresh test databases automatically after a test run. [#868](https://github.com/github/vscode-codeql/pull/868)
 
 ## 1.4.8 - 05 May 2021
 

extensions/ql-vscode/src/databases.ts

@@ -265,6 +265,11 @@ export interface DatabaseItem {
    */
   belongsToSourceArchiveExplorerUri(uri: vscode.Uri): boolean;
 
+  /**
+   * Whether the database may be affected by test execution for the given path.
+   */
+  isAffectedByTest(testPath: string): Promise<boolean>;
+
   /**
    * Gets the state of this database, to be persisted in the workspace state.
    */
@@ -470,6 +475,27 @@ export class DatabaseItemImpl implements DatabaseItem {
     return uri.scheme === zipArchiveScheme &&
       decodeSourceArchiveUri(uri).sourceArchiveZipPath === this.sourceArchive.fsPath;
   }
+
+  public async isAffectedByTest(testPath: string): Promise<boolean> {
+    const databasePath = this.databaseUri.fsPath;
+    if (!databasePath.endsWith('.testproj')) {
+      return false;
+    }
+    try {
+      const stats = await fs.stat(testPath);
+      if (stats.isDirectory()) {
+        return !path.relative(testPath, databasePath).startsWith('..');
+      } else {
+        // database for /one/two/three/test.ql is at /one/two/three/three.testproj
+        const testdir = path.dirname(testPath);
+        const testdirbase = path.basename(testdir);
+        return databasePath == path.join(testdir, testdirbase + '.testproj');
+      }
+    } catch {
+      // No information available for test path - assume database is unaffected.
+      return false;
+    }
+  }
 }
 
 /**

extensions/ql-vscode/src/extension.ts

@@ -526,7 +526,7 @@ async function activateWithInstalledDistribution(
   );
   if (testExplorerExtension) {
     const testHub = testExplorerExtension.exports;
-    const testAdapterFactory = new QLTestAdapterFactory(testHub, cliServer);
+    const testAdapterFactory = new QLTestAdapterFactory(testHub, cliServer, dbm);
     ctx.subscriptions.push(testAdapterFactory);
 
     const testUIService = new TestUIService(testHub);

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

@@ -1,3 +1,4 @@
+import * as fs from 'fs-extra';
 import * as path from 'path';
 import * as vscode from 'vscode';
 import {
@@ -17,8 +18,9 @@ import { QLTestFile, QLTestNode, QLTestDirectory, QLTestDiscovery } from './qlte
 import { Event, EventEmitter, CancellationTokenSource, CancellationToken } from 'vscode';
 import { DisposableObject } from './pure/disposable-object';
 import { CodeQLCliServer } from './cli';
-import { getOnDiskWorkspaceFolders } from './helpers';
+import { getOnDiskWorkspaceFolders, showAndLogErrorMessage, showAndLogWarningMessage } from './helpers';
 import { testLogger } from './logging';
+import { DatabaseItem, DatabaseManager } from './databases';
 
 /**
  * Get the full path of the `.expected` file for the specified QL test.
@@ -57,13 +59,13 @@ function getTestOutputFile(testPath: string, extension: string): string {
  * A factory service that creates `QLTestAdapter` objects for workspace folders on demand.
  */
 export class QLTestAdapterFactory extends DisposableObject {
-  constructor(testHub: TestHub, cliServer: CodeQLCliServer) {
+  constructor(testHub: TestHub, cliServer: CodeQLCliServer, databaseManager: DatabaseManager) {
     super();
 
     // this will register a QLTestAdapter for each WorkspaceFolder
     this.push(new TestAdapterRegistrar(
       testHub,
-      workspaceFolder => new QLTestAdapter(workspaceFolder, cliServer)
+      workspaceFolder => new QLTestAdapter(workspaceFolder, cliServer, databaseManager)
     ));
   }
 }
@@ -91,7 +93,8 @@ export class QLTestAdapter extends DisposableObject implements TestAdapter {
 
   constructor(
     public readonly workspaceFolder: vscode.WorkspaceFolder,
-    private readonly cliServer: CodeQLCliServer
+    private readonly cliServer: CodeQLCliServer,
+    private readonly databaseManager: DatabaseManager
   ) {
     super();
 
@@ -182,19 +185,79 @@ export class QLTestAdapter extends DisposableObject implements TestAdapter {
     testLogger.outputChannel.show(true);
 
     this.runningTask = this.track(new CancellationTokenSource());
+    const token = this.runningTask.token;
 
     this._testStates.fire({ type: 'started', tests: tests } as TestRunStartedEvent);
 
+    const currentDatabaseUri = this.databaseManager.currentDatabaseItem?.databaseUri;
+    const databasesUnderTest = this.databaseManager.databaseItems
+      .filter(database => tests.find(testPath => database.isAffectedByTest(testPath)));
+
+    await this.removeDatabasesBeforeTests(databasesUnderTest, token);
     try {
-      await this.runTests(tests, this.runningTask.token);
-    }
-    catch (e) {
-      /**/
+      await this.runTests(tests, token);
+    } catch (e) {
+      // CodeQL testing can throw exception even in normal scenarios. For example, if the test run
+      // produces no output (which is normal), the testing command would throw an exception on
+      // unexpected EOF during json parsing. So nothing needs to be done here - all the relevant
+      // error information (if any) should have already been written to the test logger.
     }
+    await this.reopenDatabasesAfterTests(databasesUnderTest, currentDatabaseUri, token);
+
     this._testStates.fire({ type: 'finished' } as TestRunFinishedEvent);
     this.clearTask();
   }
 
+  private async removeDatabasesBeforeTests(
+    databasesUnderTest: DatabaseItem[], token: vscode.CancellationToken): Promise<void> {
+    for (const database of databasesUnderTest) {
+      try {
+        await this.databaseManager
+          .removeDatabaseItem(_ => { /* no progress reporting */ }, token, database);
+      } catch (e) {
+        // This method is invoked from Test Explorer UI, and testing indicates that Test
+        // Explorer UI swallows any thrown exception without reporting it to the user.
+        // So we need to display the error message ourselves and then rethrow.
+        showAndLogErrorMessage(`Cannot remove database ${database.name}: ${e}`);
+        throw e;
+      }
+    }
+  }
+
+  private async reopenDatabasesAfterTests(
+    databasesUnderTest: DatabaseItem[],
+    currentDatabaseUri: vscode.Uri | undefined,
+    token: vscode.CancellationToken): Promise<void> {
+    for (const closedDatabase of databasesUnderTest) {
+      const uri = closedDatabase.databaseUri;
+      if (await this.isFileAccessible(uri)) {
+        try {
+          const reopenedDatabase = await this.databaseManager
+            .openDatabase(_ => { /* no progress reporting */ }, token, uri);
+          await this.databaseManager.renameDatabaseItem(reopenedDatabase, closedDatabase.name);
+          if (currentDatabaseUri == uri) {
+            await this.databaseManager.setCurrentDatabaseItem(reopenedDatabase, true);
+          }
+        } catch (e) {
+          // This method is invoked from Test Explorer UI, and testing indicates that Test
+          // Explorer UI swallows any thrown exception without reporting it to the user.
+          // So we need to display the error message ourselves and then rethrow.
+          showAndLogWarningMessage(`Cannot reopen database ${uri}: ${e}`);
+          throw e;
+        }
+      }
+    }
+  }
+
+  private async isFileAccessible(uri: vscode.Uri): Promise<boolean> {
+    try {
+      await fs.access(uri.fsPath);
+      return true;
+    } catch {
+      return false;
+    }
+  }
+
   private clearTask(): void {
     if (this.runningTask !== undefined) {
       const runningTask = this.runningTask;

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

@@ -179,7 +179,7 @@ describe('databases', () => {
       expect(spy).to.have.been.calledWith(mockEvent);
     });
 
-    it('should add a database item source archive', async function() {
+    it('should add a database item source archive', async function () {
       const mockDbItem = createMockDB();
       mockDbItem.name = 'xxx';
       await (databaseManager as any).addDatabaseSourceArchiveFolder(mockDbItem);
@@ -414,6 +414,72 @@ describe('databases', () => {
     expect(result).to.eq('');
   });
 
+  describe('isAffectedByTest', () => {
+    const directoryStats = new fs.Stats();
+    const fileStats = new fs.Stats();
+
+    before(() => {
+      sinon.stub(directoryStats, 'isDirectory').returns(true);
+      sinon.stub(fileStats, 'isDirectory').returns(false);
+    });
+
+    it('should return true for testproj database in test directory', async () => {
+      sandbox.stub(fs, 'stat').resolves(directoryStats);
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.testproj'));
+      expect(await db.isAffectedByTest('/path/to/dir')).to.true;
+    });
+
+    it('should return false for non-existent test directory', async () => {
+      sandbox.stub(fs, 'stat').throws('Simulated Error: ENOENT');
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.testproj'));
+      expect(await db.isAffectedByTest('/path/to/dir')).to.false;
+    });
+
+    it('should return false for non-testproj database in test directory', async () => {
+      sandbox.stub(fs, 'stat').resolves(directoryStats);
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.proj'));
+      expect(await db.isAffectedByTest('/path/to/dir')).to.false;
+    });
+
+    it('should return false for testproj database outside test directory', async () => {
+      sandbox.stub(fs, 'stat').resolves(directoryStats);
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/other/dir.testproj'));
+      expect(await db.isAffectedByTest('/path/to/dir')).to.false;
+    });
+
+    it('should return false for testproj database for prefix directory', async () => {
+      sandbox.stub(fs, 'stat').resolves(directoryStats);
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.testproj'));
+      // /path/to/d is a prefix of /path/to/dir/dir.testproj, but
+      // /path/to/dir/dir.testproj is not under /path/to/d
+      expect(await db.isAffectedByTest('/path/to/d')).to.false;
+    });
+
+    it('should return true for testproj database for test file', async () => {
+      sandbox.stub(fs, 'stat').resolves(fileStats);
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.testproj'));
+      expect(await db.isAffectedByTest('/path/to/dir/test.ql')).to.true;
+    });
+
+    it('should return false for non-existent test file', async () => {
+      sandbox.stub(fs, 'stat').throws('Simulated Error: ENOENT');
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.testproj'));
+      expect(await db.isAffectedByTest('/path/to/dir/test.ql')).to.false;
+    });
+
+    it('should return false for non-testproj database for test file', async () => {
+      sandbox.stub(fs, 'stat').resolves(fileStats);
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.proj'));
+      expect(await db.isAffectedByTest('/path/to/dir/test.ql')).to.false;
+    });
+
+    it('should return false for testproj database not matching test file', async () => {
+      sandbox.stub(fs, 'stat').resolves(fileStats);
+      const db = createMockDB(sourceLocationUri(), Uri.file('/path/to/dir/dir.testproj'));
+      expect(await db.isAffectedByTest('/path/to/test.ql')).to.false;
+    });
+  });
+
   function createMockDB(
     // source archive location must be a real(-ish) location since
     // tests will add this to the workspace location

extensions/ql-vscode/src/vscode-tests/no-workspace/test-adapter.test.ts

@@ -1,32 +1,70 @@
 import 'vscode-test';
 import 'mocha';
 import * as sinon from 'sinon';
+import * as fs from 'fs-extra';
 import { Uri, WorkspaceFolder } from 'vscode';
 import { expect } from 'chai';
 
 import { QLTestAdapter } from '../../test-adapter';
 import { CodeQLCliServer } from '../../cli';
+import { DatabaseItem, DatabaseItemImpl, DatabaseManager, FullDatabaseOptions } from '../../databases';
 
 describe('test-adapter', () => {
   let adapter: QLTestAdapter;
+  let fakeDatabaseManager: DatabaseManager;
+  let currentDatabaseItem: DatabaseItem | undefined;
+  let databaseItems: DatabaseItem[] = [];
+  let openDatabaseSpy: sinon.SinonStub;
+  let removeDatabaseItemSpy: sinon.SinonStub;
+  let renameDatabaseItemSpy: sinon.SinonStub;
+  let setCurrentDatabaseItemSpy: sinon.SinonStub;
   let runTestsSpy: sinon.SinonStub;
   let resolveTestsSpy: sinon.SinonStub;
   let resolveQlpacksSpy: sinon.SinonStub;
   let sandox: sinon.SinonSandbox;
 
+  const preTestDatabaseItem = new DatabaseItemImpl(
+    Uri.file('/path/to/test/dir/dir.testproj'),
+    undefined,
+    { displayName: 'custom display name' } as unknown as FullDatabaseOptions,
+    (_) => { /* no change event listener */ }
+  );
+  const postTestDatabaseItem = new DatabaseItemImpl(
+    Uri.file('/path/to/test/dir/dir.testproj'),
+    undefined,
+    { displayName: 'default name' } as unknown as FullDatabaseOptions,
+    (_) => { /* no change event listener */ }
+  );
+
   beforeEach(() => {
     sandox = sinon.createSandbox();
     mockRunTests();
+    openDatabaseSpy = sandox.stub().resolves(postTestDatabaseItem);
+    removeDatabaseItemSpy = sandox.stub().resolves();
+    renameDatabaseItemSpy = sandox.stub().resolves();
+    setCurrentDatabaseItemSpy = sandox.stub().resolves();
     resolveQlpacksSpy = sandox.stub().resolves({});
     resolveTestsSpy = sandox.stub().resolves([]);
+    fakeDatabaseManager = {
+      currentDatabaseItem: undefined,
+      databaseItems: undefined,
+      openDatabase: openDatabaseSpy,
+      removeDatabaseItem: removeDatabaseItemSpy,
+      renameDatabaseItem: renameDatabaseItemSpy,
+      setCurrentDatabaseItem: setCurrentDatabaseItemSpy,
+    } as unknown as DatabaseManager;
+    sandox.stub(fakeDatabaseManager, 'currentDatabaseItem').get(() => currentDatabaseItem);
+    sandox.stub(fakeDatabaseManager, 'databaseItems').get(() => databaseItems);
+    sandox.stub(preTestDatabaseItem, 'isAffectedByTest').resolves(true);
     adapter = new QLTestAdapter({
       name: 'ABC',
       uri: Uri.parse('file:/ab/c')
     } as WorkspaceFolder, {
       runTests: runTestsSpy,
       resolveQlpacks: resolveQlpacksSpy,
       resolveTests: resolveTestsSpy
-    } as unknown as CodeQLCliServer);
+    } as unknown as CodeQLCliServer,
+      fakeDatabaseManager);
   });
 
   afterEach(() => {
@@ -74,12 +112,33 @@ describe('test-adapter', () => {
     expect(listenerSpy).to.have.callCount(5);
   });
 
+  it('should reregister testproj databases around test run', async () => {
+    sandox.stub(fs, 'access').resolves();
+    currentDatabaseItem = preTestDatabaseItem;
+    databaseItems = [preTestDatabaseItem];
+    await adapter.run(['/path/to/test/dir']);
+
+    removeDatabaseItemSpy.getCall(0).calledBefore(runTestsSpy.getCall(0));
+    openDatabaseSpy.getCall(0).calledAfter(runTestsSpy.getCall(0));
+    renameDatabaseItemSpy.getCall(0).calledAfter(openDatabaseSpy.getCall(0));
+    setCurrentDatabaseItemSpy.getCall(0).calledAfter(openDatabaseSpy.getCall(0));
+
+    sinon.assert.calledOnceWithExactly(
+      removeDatabaseItemSpy, sinon.match.any, sinon.match.any, preTestDatabaseItem);
+    sinon.assert.calledOnceWithExactly(
+      openDatabaseSpy, sinon.match.any, sinon.match.any, preTestDatabaseItem.databaseUri);
+    sinon.assert.calledOnceWithExactly(
+      renameDatabaseItemSpy, postTestDatabaseItem, preTestDatabaseItem.name);
+    sinon.assert.calledOnceWithExactly(
+      setCurrentDatabaseItemSpy, postTestDatabaseItem, true);
+  });
+
   function mockRunTests() {
     // runTests is an async generator function. This is not directly supported in sinon
     // However, we can pretend the same thing by just returning an async array.
     runTestsSpy = sandox.stub();
     runTestsSpy.returns(
-      (async function*() {
+      (async function* () {
         yield Promise.resolve({
           test: Uri.parse('file:/ab/c/d.ql').fsPath,
           pass: true,