Merge pull request #2538 from github/dbartol/save-before-start

Save dirty documents before evaluating queries
This commit is contained in:
Dave Bartolomeo
2023-07-11 15:55:03 -04:00
committed by GitHub
19 changed files with 219 additions and 110 deletions

View File

@@ -71,6 +71,7 @@
"contributes": {
"configurationDefaults": {
"[ql]": {
"debug.saveBeforeStart": "nonUntitledEditorsInActiveGroup",
"editor.wordBasedSuggestions": false
},
"[dbscheme]": {
@@ -246,8 +247,8 @@
},
"codeQL.runningQueries.autoSave": {
"type": "boolean",
"default": false,
"description": "Enable automatically saving a modified query file when running a query."
"description": "Enable automatically saving a modified query file when running a query.",
"markdownDeprecationMessage": "This property is deprecated and no longer has any effect. To control automatic saving of documents before running queries, use the `debug.saveBeforeStart` setting."
},
"codeQL.runningQueries.maxQueries": {
"type": "integer",
@@ -1726,7 +1727,7 @@
"test:vscode-integration:activated-extension": "jest --projects test/vscode-tests/activated-extension",
"test:vscode-integration:no-workspace": "jest --projects test/vscode-tests/no-workspace",
"test:vscode-integration:minimal-workspace": "jest --projects test/vscode-tests/minimal-workspace",
"test:cli-integration": "jest --projects test/vscode-tests/cli-integration",
"test:cli-integration": "jest --projects test/vscode-tests/cli-integration --verbose",
"update-vscode": "node ./node_modules/vscode/bin/install",
"format": "prettier --write **/*.{ts,tsx} && eslint . --ext .ts,.tsx --fix",
"lint": "eslint . --ext .js,.ts,.tsx --max-warnings=0",

View File

@@ -70,6 +70,12 @@ export interface InspectionResult<T> {
workspaceFolderValue?: T;
}
const VSCODE_DEBUG_SETTING = new Setting("debug", undefined);
export const VSCODE_SAVE_BEFORE_START_SETTING = new Setting(
"saveBeforeStart",
VSCODE_DEBUG_SETTING,
);
const ROOT_SETTING = new Setting("codeQL");
// Global configuration
@@ -161,10 +167,6 @@ export const NUMBER_OF_TEST_THREADS_SETTING = new Setting(
RUNNING_TESTS_SETTING,
);
export const MAX_QUERIES = new Setting("maxQueries", RUNNING_QUERIES_SETTING);
export const AUTOSAVE_SETTING = new Setting(
"autoSave",
RUNNING_QUERIES_SETTING,
);
export const PAGE_SIZE = new Setting("pageSize", RESULTS_DISPLAY_SETTING);
const CUSTOM_LOG_DIRECTORY_SETTING = new Setting(
"customLogDirectory",

View File

@@ -14,6 +14,7 @@ import { CoreQueryResults } from "../query-server";
import {
getQuickEvalContext,
QueryOutputDir,
saveBeforeStart,
validateQueryUri,
} from "../run-queries-shared";
import { QLResolvedDebugConfiguration } from "./debug-configuration";
@@ -73,6 +74,9 @@ class QLDebugAdapterTracker
}
public async quickEval(): Promise<void> {
// Since we're not going through VS Code's launch path, we need to save dirty files ourselves.
await saveBeforeStart();
const args: CodeQLProtocol.QuickEvalRequest["arguments"] = {
quickEvalContext: await getQuickEvalContext(undefined, false),
};

View File

@@ -10,7 +10,6 @@ import {
Range,
Uri,
window,
workspace,
} from "vscode";
import {
TeeLogger,
@@ -32,8 +31,8 @@ import {
createInitialQueryInfo,
createTimestampFile,
getQuickEvalContext,
promptUserToSaveChanges,
QueryOutputDir,
saveBeforeStart,
SelectedQuery,
validateQueryUri,
} from "../run-queries-shared";
@@ -54,25 +53,6 @@ interface DatabaseQuickPickItem extends QuickPickItem {
databaseItem: DatabaseItem;
}
/**
* If either the query file or the quickeval file is dirty, give the user the chance to save them.
*/
async function promptToSaveQueryIfNeeded(query: SelectedQuery): Promise<void> {
// There seems to be no way to ask VS Code to find an existing text document by name, without
// automatically opening the document if it is not found.
const queryUri = Uri.file(query.queryPath).toString();
const quickEvalUri =
query.quickEval !== undefined
? Uri.file(query.quickEval.quickEvalPosition.fileName).toString()
: undefined;
for (const openDocument of workspace.textDocuments) {
const documentUri = openDocument.uri.toString();
if (documentUri === queryUri || documentUri === quickEvalUri) {
await promptUserToSaveChanges(openDocument);
}
}
}
export enum QuickEvalType {
None,
QuickEval,
@@ -432,6 +412,8 @@ export class LocalQueries extends DisposableObject {
range?: Range,
templates?: Record<string, string>,
): Promise<CoreCompletedQuery> {
await saveBeforeStart();
let queryPath: string;
if (queryUri !== undefined) {
// The query URI is provided by the command, most likely because the command was run from an
@@ -462,8 +444,6 @@ export class LocalQueries extends DisposableObject {
const additionalPacks = getOnDiskWorkspaceFolders();
const extensionPacks = await this.getDefaultExtensionPacks(additionalPacks);
await promptToSaveQueryIfNeeded(selectedQuery);
const coreQueryRun = this.queryRunner.createQueryRun(
databaseItem.databaseUri.fsPath,
{

View File

@@ -2,16 +2,8 @@ import * as messages from "./query-server/messages-shared";
import * as legacyMessages from "./query-server/legacy-messages";
import { DatabaseInfo, QueryMetadata } from "./common/interface-types";
import { join, parse, dirname, basename } from "path";
import {
ConfigurationTarget,
Range,
TextDocument,
TextEditor,
Uri,
window,
} from "vscode";
import { isCanary, AUTOSAVE_SETTING } from "./config";
import { UserCancellationException } from "./common/vscode/progress";
import { Range, TextEditor, Uri, window, workspace } from "vscode";
import { isCanary, VSCODE_SAVE_BEFORE_START_SETTING } from "./config";
import {
pathExists,
readFile,
@@ -491,55 +483,41 @@ async function getSelectedPosition(
};
}
type SaveBeforeStartMode =
| "nonUntitledEditorsInActiveGroup"
| "allEditorsInActiveGroup"
| "none";
/**
* Prompts the user to save `document` if it has unsaved changes.
*
* @param document The document to save.
*
* @returns true if we should save changes and false if we should continue without saving changes.
* @throws UserCancellationException if we should abort whatever operation triggered this prompt
* Saves dirty files before running queries, based on the user's settings.
*/
export async function promptUserToSaveChanges(
document: TextDocument,
): Promise<boolean> {
if (document.isDirty) {
if (AUTOSAVE_SETTING.getValue()) {
return true;
} else {
const yesItem = { title: "Yes", isCloseAffordance: false };
const alwaysItem = { title: "Always Save", isCloseAffordance: false };
const noItem = {
title: "No (run version on disk)",
isCloseAffordance: false,
};
const cancelItem = { title: "Cancel", isCloseAffordance: true };
const message = `Query file '${basename(
document.uri.fsPath,
)}' has unsaved changes. Save now?`;
const chosenItem = await window.showInformationMessage(
message,
{ modal: true },
yesItem,
alwaysItem,
noItem,
cancelItem,
);
export async function saveBeforeStart(): Promise<void> {
const mode: SaveBeforeStartMode =
(VSCODE_SAVE_BEFORE_START_SETTING.getValue<string>({
languageId: "ql",
}) as SaveBeforeStartMode) ?? "nonUntitledEditorsInActiveGroup";
if (chosenItem === alwaysItem) {
await AUTOSAVE_SETTING.updateValue(true, ConfigurationTarget.Workspace);
return true;
}
// Despite the names of the modes, the VS Code implementation doesn't restrict itself to the
// current tab group. It saves all dirty files in all groups. We'll do the same.
switch (mode) {
case "nonUntitledEditorsInActiveGroup":
await workspace.saveAll(false);
break;
if (chosenItem === yesItem) {
return true;
}
case "allEditorsInActiveGroup":
// The VS Code implementation of this mode only saves an untitled file if it is the document
// in the active editor. That's too much work for us, so we'll just live with the inconsistency.
await workspace.saveAll(true);
break;
if (chosenItem === cancelItem) {
throw new UserCancellationException("Query run cancelled.", true);
}
}
case "none":
break;
default:
// Unexpected value. Fall back to the default behavior.
await workspace.saveAll(false);
break;
}
return false;
}
/**
@@ -566,7 +544,7 @@ async function convertToQlPath(filePath: string): Promise<string> {
}
}
}
throw new Error(`Can't convert path to form suitable for QL:${filePath}`);
throw new Error(`Can't convert path to form suitable for QL: ${filePath}`);
} else {
return filePath;
}

View File

@@ -0,0 +1,20 @@
---
lockVersion: 1.0.0
dependencies:
codeql/javascript-all:
version: 0.6.3
codeql/javascript-queries:
version: 0.6.3
codeql/regex:
version: 0.0.14
codeql/suite-helpers:
version: 0.5.3
codeql/tutorial:
version: 0.0.11
codeql/typos:
version: 0.0.18
codeql/util:
version: 0.0.11
codeql/yaml:
version: 0.0.3
compiled: false

View File

@@ -0,0 +1,4 @@
name: integration-test-debugger-javascript
version: 0.0.0
dependencies:
codeql/javascript-queries: "*"

View File

@@ -57,11 +57,16 @@ describe("files", () => {
it("should scan a directory", async () => {
const file1 = join(dataDir, "compute-default-strings.ql");
const file2 = join(dataDir, "multiple-result-sets.ql");
const file3 = join(dataDir, "query.ql");
const file2 = join(dataDir, "debugger", "QuickEvalQuery.ql");
const file3 = join(dataDir, "debugger", "simple-query.ql");
const file4 = join(dataDir, "multiple-result-sets.ql");
const file5 = join(dataDir, "query.ql");
const result = await gatherQlFiles([dataDir]);
expect(result.sort()).toEqual([[file1, file2, file3], true]);
expect(result.sort()).toEqual([
[file1, file2, file3, file4, file5],
true,
]);
});
it("should scan a directory and some files", async () => {
@@ -78,12 +83,17 @@ describe("files", () => {
it("should avoid duplicates", async () => {
const file1 = join(dataDir, "compute-default-strings.ql");
const file2 = join(dataDir, "multiple-result-sets.ql");
const file3 = join(dataDir, "query.ql");
const file2 = join(dataDir, "debugger", "QuickEvalQuery.ql");
const file3 = join(dataDir, "debugger", "simple-query.ql");
const file4 = join(dataDir, "multiple-result-sets.ql");
const file5 = join(dataDir, "query.ql");
const result = await gatherQlFiles([file1, dataDir, file3]);
const result = await gatherQlFiles([file1, dataDir, file3, file4, file5]);
result[0].sort();
expect(result.sort()).toEqual([[file1, file2, file3], true]);
expect(result.sort()).toEqual([
[file1, file2, file3, file4, file5],
true,
]);
});
});

View File

@@ -3,6 +3,7 @@ import { join, resolve } from "path";
import { CodeQLCliServer } from "../../../../src/codeql-cli/cli";
import { getActivatedExtension } from "../../global.helper";
import { tryGetQueryMetadata } from "../../../../src/codeql-cli/query-metadata";
import { getDataFolderFilePath } from "../utils";
describe("tryGetQueryMetadata", () => {
const baseDir = resolve(__dirname, "..");
@@ -30,7 +31,7 @@ describe("tryGetQueryMetadata", () => {
// Query with empty metadata
const noMetadata = await tryGetQueryMetadata(
cli,
join(baseDir, "data", "simple-query.ql"),
getDataFolderFilePath("debugger/simple-query.ql"),
);
expect(noMetadata).toEqual({});

View File

@@ -1,10 +1,20 @@
---
lockVersion: 1.0.0
dependencies:
codeql-javascript:
version: 0.5.1
codeql/javascript-all:
version: 0.6.3
codeql/javascript-queries:
version: 0.6.3
codeql/regex:
version: 0.0.9
version: 0.0.14
codeql/suite-helpers:
version: 0.5.3
codeql/tutorial:
version: 0.0.6
version: 0.0.11
codeql/typos:
version: 0.0.18
codeql/util:
version: 0.0.11
codeql/yaml:
version: 0.0.3
compiled: false

View File

@@ -1,5 +1,11 @@
import { Selection, Uri, window, workspace } from "vscode";
import { join } from "path";
import {
Position,
Selection,
TextEditor,
Uri,
window,
workspace,
} from "vscode";
import { DatabaseManager } from "../../../../src/databases/local-databases";
import {
@@ -13,6 +19,7 @@ import { CodeQLCliServer } from "../../../../src/codeql-cli/cli";
import { QueryOutputDir } from "../../../../src/run-queries-shared";
import { createVSCodeCommandManager } from "../../../../src/common/vscode/commands";
import { AllCommands } from "../../../../src/common/commands";
import { getDataFolderFilePath } from "../utils";
async function selectForQuickEval(
path: string,
@@ -20,10 +27,12 @@ async function selectForQuickEval(
column: number,
endLine: number,
endColumn: number,
): Promise<void> {
): Promise<TextEditor> {
const document = await workspace.openTextDocument(path);
const editor = await window.showTextDocument(document);
editor.selection = new Selection(line, column, endLine, endColumn);
return editor;
}
async function getResultCount(
@@ -42,9 +51,11 @@ describeWithCodeQL()("Debugger", () => {
let databaseManager: DatabaseManager;
let cli: CodeQLCliServer;
const appCommands = createVSCodeCommandManager<AllCommands>();
const simpleQueryPath = join(__dirname, "..", "data", "simple-query.ql");
const quickEvalQueryPath = join(__dirname, "..", "data", "QuickEvalQuery.ql");
const quickEvalLibPath = join(__dirname, "..", "data", "QuickEvalLib.qll");
const simpleQueryPath = getDataFolderFilePath("debugger/simple-query.ql");
const quickEvalQueryPath = getDataFolderFilePath(
"debugger/QuickEvalQuery.ql",
);
const quickEvalLibPath = getDataFolderFilePath("debugger/QuickEvalLib.qll");
beforeEach(async () => {
const extension = await getActivatedExtension();
@@ -138,4 +149,32 @@ describeWithCodeQL()("Debugger", () => {
await controller.expectStopped();
});
});
it("should save dirty documents before launching a debug session", async () => {
await withDebugController(appCommands, async (controller) => {
const editor = await selectForQuickEval(quickEvalLibPath, 4, 15, 4, 32);
expect(editor.document.isDirty).toBe(false);
await controller.startDebuggingSelection({
query: quickEvalQueryPath, // The query context. This query extends the abstract class.
});
await controller.expectLaunched();
// Should have saved the dirty document.
expect(editor.document.isDirty).toBe(false);
await controller.expectSucceeded();
await controller.expectStopped();
await editor.edit((editBuilder) => {
editBuilder.insert(new Position(0, 0), "/* another comment */");
});
expect(editor.document.isDirty).toBe(true);
await controller.continueDebuggingSelection();
await controller.expectSucceeded();
await controller.expectStopped();
expect(editor.document.isDirty).toBe(false);
});
});
});

View File

@@ -1,10 +1,19 @@
const path = require("path");
const fs = require("fs");
const {
config: baseConfig,
tmpDir,
rootDir,
} = require("../jest-runner-vscode.config.base");
// Copy the workspace content to a temporary directory, and open it there. Some of our tests write
// to files in the workspace, so we don't want to do that in the source directory.
const tmpDataDir = path.join(tmpDir.name, "data");
fs.cpSync(path.resolve(rootDir, "test/data"), tmpDataDir, {
recursive: true,
});
/** @type import("jest-runner-vscode").RunnerOptions */
const config = {
...baseConfig,
@@ -17,7 +26,8 @@ const config = {
"github.codespaces",
"--disable-extension",
"github.copilot",
path.resolve(rootDir, "test/data"),
"--disable-workspace-trust", // Disable trust because we copy our workspace to a temp directory
tmpDataDir,
path.resolve(rootDir, "test/data-extensions"), // folder containing the extension packs and packs that are targeted by the extension pack
// CLI integration tests requires a multi-root workspace so that the data and the QL sources are accessible.
...(process.env.TEST_CODEQL_PATH ? [process.env.TEST_CODEQL_PATH] : []),

View File

@@ -38,6 +38,9 @@ import {
} from "../../../src/common/commands";
import { ProgressCallback } from "../../../src/common/vscode/progress";
import { withDebugController } from "./debugger/debug-controller";
import { getDataFolderFilePath } from "./utils";
const simpleQueryPath = getDataFolderFilePath("debugger/simple-query.ql");
type DebugMode = "localQueries" | "debug";
@@ -71,6 +74,7 @@ async function compileAndRunQuery(
},
true,
);
await controller.expectLaunched();
const succeeded = await controller.expectSucceeded();
await controller.expectExited();
@@ -164,8 +168,11 @@ describeWithCodeQL()("Queries", () => {
return;
}
console.log(`Starting 'extensions' ${mode}`);
console.log("Setting useExtensionPacks to true");
await cli.setUseExtensionPacks(true);
const parsedResults = await runQueryWithExtensions();
console.log("Returned from runQueryWithExtensions");
expect(parsedResults).toEqual([1, 2, 3, 4]);
});
@@ -180,6 +187,7 @@ describeWithCodeQL()("Queries", () => {
}
async function runQueryWithExtensions() {
console.log("Calling compileAndRunQuery");
const result = await compileAndRunQuery(
mode,
appCommandManager,
@@ -191,10 +199,12 @@ describeWithCodeQL()("Queries", () => {
dbItem,
undefined,
);
console.log("Completed compileAndRunQuery");
// Check that query was successful
expect(result.resultType).toBe(QueryResultType.SUCCESS);
console.log("Loading query results");
// Load query results
const chunk = await qs.cliServer.bqrsDecode(
result.outputDir.bqrsPath,
@@ -205,6 +215,7 @@ describeWithCodeQL()("Queries", () => {
pageSize: 10,
},
);
console.log("Loaded query results");
// Extract the results as an array.
return chunk.tuples.map((t) => t[0]);
@@ -213,13 +224,12 @@ describeWithCodeQL()("Queries", () => {
describe.each(MODES)("running queries (%s)", (mode) => {
it("should run a query", async () => {
const queryPath = join(__dirname, "data", "simple-query.ql");
const result = await compileAndRunQuery(
mode,
appCommandManager,
localQueries,
QuickEvalType.None,
Uri.file(queryPath),
Uri.file(simpleQueryPath),
progress,
token,
dbItem,
@@ -233,13 +243,12 @@ describeWithCodeQL()("Queries", () => {
// Asserts a fix for bug https://github.com/github/vscode-codeql/issues/733
it("should restart the database and run a query", async () => {
await appCommandManager.execute("codeQL.restartQueryServer");
const queryPath = join(__dirname, "data", "simple-query.ql");
const result = await compileAndRunQuery(
mode,
appCommandManager,
localQueries,
QuickEvalType.None,
Uri.file(queryPath),
Uri.file(simpleQueryPath),
progress,
token,
dbItem,

View File

@@ -0,0 +1,9 @@
import { workspace } from "vscode";
import { join } from "path";
/**
* Get the absolute path to a file in the temporary copy of the `data` folder.
*/
export function getDataFolderFilePath(path: string): string {
return join(workspace.workspaceFolders![0].uri.fsPath, path);
}

View File

@@ -1,7 +1,12 @@
const path = require("path");
const os = require("os");
const tmp = require("tmp-promise");
const tmpDir = tmp.dirSync({ unsafeCleanup: true });
// Use the Actions runner temp dir if available, otherwise use the system temp dir
// On Actions runners, we can get into Windows "long path" territory if we use the
// system temp dir.
const overrideTmpDir = process.env.RUNNER_TEMP || os.tmpdir();
const tmpDir = tmp.dirSync({ unsafeCleanup: true, tmpdir: overrideTmpDir });
const rootDir = path.resolve(__dirname, "../..");

View File

@@ -3,6 +3,7 @@ import {
ConfigurationTarget,
workspace,
WorkspaceConfiguration as VSCodeWorkspaceConfiguration,
Uri,
} from "vscode";
import { readFileSync } from "fs-extra";
import { join } from "path";
@@ -151,7 +152,14 @@ const packageConfiguration: Record<
const pkg = JSON.parse(
readFileSync(join(__dirname, "../../package.json"), "utf-8"),
);
return pkg.contributes.configuration.properties;
return {
...pkg.contributes.configuration.properties,
// `debug.saveBeforeStart` is a core VS Code setting, but we depend on its value in these tests.
// We'll set it here to the value that we expect.
"debug.saveBeforeStart": {
default: "nonUntitledEditorsInActiveGroup",
},
};
})();
export const vsCodeGetConfiguration = workspace.getConfiguration;
@@ -159,6 +167,25 @@ export let vscodeGetConfigurationMock: jest.SpiedFunction<
typeof workspace.getConfiguration
>;
function acceptScope(scope: ConfigurationScope | null | undefined): boolean {
if (!scope) {
return true;
}
if (scope instanceof Uri) {
return false;
}
// Reject any scope that has a URI property. That covers `WorkspaceFolder`, `TextDocument`, and any
if (scope.uri !== undefined) {
return false;
}
// We're left with only `{ languageId }` scopes. We'll ignore the language, since it doesn't matter
// for our tests.
return true;
}
export const beforeEachAction = async () => {
const defaultConfiguration = new DefaultConfiguration(packageConfiguration);
@@ -176,7 +203,7 @@ export const beforeEachAction = async () => {
section?: string,
scope?: ConfigurationScope | null,
): VSCodeWorkspaceConfiguration => {
if (scope) {
if (!acceptScope(scope)) {
throw new Error("Scope is not supported in tests");
}