Skip to content

Remove more instances of as unknown as #2103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
koesie10 merged 7 commits into from
Feb 28, 2023
Merged

Remove more instances of as unknown as #2103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fa5bad6
Remove `as unknown as TextEditor/TextDocument`
koesie10 Feb 21, 2023
9b1ca51
Remove `as unknown as WorkspaceFolder`
koesie10 Feb 21, 2023
551ed95
Remove `as unknown as QueryHistoryManager`
koesie10 Feb 21, 2023
af167c6
Remove `as unknown as DatabaseManager`
koesie10 Feb 21, 2023
dd4df01
Remove `as unknown as ExtensionContext`
koesie10 Feb 21, 2023
fd2b91d
Remove casting to `QueryHistoryConfig`
koesie10 Feb 21, 2023
65e652b
Fix `mockResolvedValue` not working with mocked objects
koesie10 Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions ...scode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import { DbManager } from "../../../../src/databases/db-manager";
import { App } from "../../../../src/common/app";
import { ExtensionApp } from "../../../../src/common/vscode/vscode-app";
import { DbConfigStore } from "../../../../src/databases/config/db-config-store";
import { mockedObject } from "../../utils/mocking.helpers";

// up to 3 minutes per test
jest.setTimeout(3 * 60 * 1000);
Expand Down Expand Up @@ -992,10 +993,10 @@ describe("Variant Analysis Manager", () => {

showTextDocumentSpy = jest
.spyOn(window, "showTextDocument")
.mockResolvedValue(undefined as unknown as TextEditor);
.mockResolvedValue(mockedObject<TextEditor>({}));
openTextDocumentSpy = jest
.spyOn(workspace, "openTextDocument")
.mockResolvedValue(undefined as unknown as TextDocument);
.mockResolvedValue(mockedObject<TextDocument>({}));
});

afterEach(() => {
Expand All @@ -1005,8 +1006,8 @@ describe("Variant Analysis Manager", () => {
it("opens the query text", async () => {
await variantAnalysisManager.openQueryText(variantAnalysis.id);

expect(showTextDocumentSpy).toHaveBeenCalledTimes(1);
expect(openTextDocumentSpy).toHaveBeenCalledTimes(1);
expect(showTextDocumentSpy).toHaveBeenCalledTimes(1);

const uri: Uri = openTextDocumentSpy.mock.calls[0][0] as Uri;
expect(uri.scheme).toEqual("codeql-variant-analysis");
Expand Down Expand Up @@ -1040,10 +1041,10 @@ describe("Variant Analysis Manager", () => {

showTextDocumentSpy = jest
.spyOn(window, "showTextDocument")
.mockResolvedValue(undefined as unknown as TextEditor);
.mockResolvedValue(mockedObject<TextEditor>({}));
openTextDocumentSpy = jest
.spyOn(workspace, "openTextDocument")
.mockResolvedValue(undefined as unknown as TextDocument);
.mockResolvedValue(mockedObject<TextDocument>({}));
});

afterEach(() => {
Expand Down
28 changes: 19 additions & 9 deletions extensions/ql-vscode/test/vscode-tests/minimal-workspace/local-databases.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ describe("local databases", () => {
>;

let dir: tmp.DirResult;
let extensionContextStoragePath: string;

beforeEach(() => {
dir = tmp.dirSync();
Expand All @@ -65,16 +66,24 @@ describe("local databases", () => {
.spyOn(helpers, "showBinaryChoiceDialog")
.mockResolvedValue(true);

extensionContext = {
workspaceState: {
update: updateSpy,
get: () => [],
extensionContextStoragePath = dir.name;

extensionContext = mockedObject<ExtensionContext>(
{
workspaceState: {
update: updateSpy,
get: () => [],
},
},
// pretend like databases added in the temp dir are controlled by the extension
// so that they are deleted upon removal
storagePath: dir.name,
storageUri: Uri.parse(dir.name),
} as unknown as ExtensionContext;
{
dynamicProperties: {
// pretend like databases added in the temp dir are controlled by the extension
// so that they are deleted upon removal
storagePath: () => extensionContextStoragePath,
storageUri: () => Uri.parse(extensionContextStoragePath),
},
},
);

databaseManager = new DatabaseManager(
extensionContext,
Expand Down Expand Up @@ -267,6 +276,7 @@ describe("local databases", () => {

// pretend that the database location is not controlled by the extension
(databaseManager as any).ctx.storagePath = "hucairz";
extensionContextStoragePath = "hucairz";

await databaseManager.removeDatabaseItem(
{} as ProgressCallback,
Expand Down
5 changes: 3 additions & 2 deletions extensions/ql-vscode/test/vscode-tests/minimal-workspace/qltest-discovery.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { DirectoryResult } from "tmp-promise";
import * as tmp from "tmp-promise";

import "../../matchers/toEqualPath";
import { mockedObject } from "../utils/mocking.helpers";

describe("qltest-discovery", () => {
describe("discoverTests", () => {
Expand Down Expand Up @@ -34,10 +35,10 @@ describe("qltest-discovery", () => {
iFile = join(hDir, "i.ql");

qlTestDiscover = new QLTestDiscovery(
{
mockedObject<WorkspaceFolder>({
uri: baseUri,
name: "My tests",
} as unknown as WorkspaceFolder,
}),
{
resolveTests() {
return [dFile, eFile, iFile];
Expand Down
4 changes: 3 additions & 1 deletion ...l-vscode/test/vscode-tests/no-workspace/query-history/history-item-label-provider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ describe("HistoryItemLabelProvider", () => {
beforeEach(() => {
config = {
format: "xxx %q xxx",
} as unknown as QueryHistoryConfig;
ttlInMillis: 0,
onDidChangeConfiguration: jest.fn(),
};
labelProvider = new HistoryItemLabelProvider(config);
});

Expand Down
17 changes: 10 additions & 7 deletions ...ql-vscode/test/vscode-tests/no-workspace/query-history/history-tree-data-provider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ import { join } from "path";
import * as vscode from "vscode";

import { extLogger } from "../../../../src/common";
import {
QueryHistoryConfig,
QueryHistoryConfigListener,
} from "../../../../src/config";
import { QueryHistoryConfigListener } from "../../../../src/config";
import { LocalQueryInfo } from "../../../../src/query-results";
import { DatabaseManager } from "../../../../src/local-databases";
import { tmpDir } from "../../../../src/helpers";
Expand Down Expand Up @@ -121,8 +118,10 @@ describe("HistoryTreeDataProvider", () => {
]);

labelProvider = new HistoryItemLabelProvider({
/**/
} as QueryHistoryConfig);
format: "",
ttlInMillis: 0,
onDidChangeConfiguration: jest.fn(),
});
historyTreeDataProvider = new HistoryTreeDataProvider(labelProvider);
});

Expand Down Expand Up @@ -432,7 +431,11 @@ describe("HistoryTreeDataProvider", () => {
extensionPath: vscode.Uri.file("/x/y/z").fsPath,
} as vscode.ExtensionContext,
configListener,
new HistoryItemLabelProvider({} as QueryHistoryConfig),
new HistoryItemLabelProvider({
format: "",
ttlInMillis: 0,
onDidChangeConfiguration: jest.fn(),
}),
doCompareCallback,
);
(qhm.treeDataProvider as any).history = [...allHistory];
Expand Down
14 changes: 8 additions & 6 deletions ...ions/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ import * as vscode from "vscode";

import { extLogger } from "../../../../src/common";
import { QueryHistoryManager } from "../../../../src/query-history/query-history-manager";
import {
QueryHistoryConfig,
QueryHistoryConfigListener,
} from "../../../../src/config";
import { QueryHistoryConfigListener } from "../../../../src/config";
import { LocalQueryInfo } from "../../../../src/query-results";
import { DatabaseManager } from "../../../../src/local-databases";
import { tmpDir } from "../../../../src/helpers";
Expand All @@ -28,6 +25,7 @@ import { VariantAnalysisStatus } from "../../../../src/variant-analysis/shared/v
import { QuickPickItem, TextEditor } from "vscode";
import { WebviewReveal } from "../../../../src/interface-utils";
import * as helpers from "../../../../src/helpers";
import { mockedObject } from "../../utils/mocking.helpers";

describe("QueryHistoryManager", () => {
const mockExtensionLocation = join(tmpDir.name, "mock-extension-location");
Expand Down Expand Up @@ -58,7 +56,7 @@ describe("QueryHistoryManager", () => {
beforeEach(() => {
showTextDocumentSpy = jest
.spyOn(vscode.window, "showTextDocument")
.mockResolvedValue(undefined as unknown as TextEditor);
.mockResolvedValue(mockedObject<TextEditor>({}));
showInformationMessageSpy = jest
.spyOn(vscode.window, "showInformationMessage")
.mockResolvedValue(undefined);
Expand Down Expand Up @@ -1158,7 +1156,11 @@ describe("QueryHistoryManager", () => {
extensionPath: vscode.Uri.file("/x/y/z").fsPath,
} as vscode.ExtensionContext,
configListener,
new HistoryItemLabelProvider({} as QueryHistoryConfig),
new HistoryItemLabelProvider({
format: "",
ttlInMillis: 0,
onDidChangeConfiguration: jest.fn(),
}),
doCompareCallback,
);
(qhm.treeDataProvider as any).history = [...allHistory];
Expand Down
5 changes: 3 additions & 2 deletions ...ons/ql-vscode/test/vscode-tests/no-workspace/query-history/query-history-scrubber.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
THREE_HOURS_IN_MS,
TWO_HOURS_IN_MS,
} from "../../../../src/pure/time";
import { mockedObject } from "../../utils/mocking.helpers";

describe("query history scrubber", () => {
const now = Date.now();
Expand Down Expand Up @@ -181,11 +182,11 @@ describe("query history scrubber", () => {
TWO_HOURS_IN_MS,
LESS_THAN_ONE_DAY,
dir,
{
mockedObject<QueryHistoryManager>({
removeDeletedQueries: () => {
return Promise.resolve();
},
} as QueryHistoryManager,
}),
mockCtx,
{
increment: () => runCount++,
Expand Down
17 changes: 12 additions & 5 deletions ...s/ql-vscode/test/vscode-tests/no-workspace/query-history/variant-analysis-history.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
import { join } from "path";

import { commands, ExtensionContext, Uri } from "vscode";
import { QueryHistoryConfig } from "../../../../src/config";
import { DatabaseManager } from "../../../../src/local-databases";
import { tmpDir, walkDirectory } from "../../../../src/helpers";
import { DisposableBucket } from "../../disposable-bucket";
Expand All @@ -20,6 +19,7 @@ import { EvalLogViewer } from "../../../../src/eval-log-viewer";
import { QueryRunner } from "../../../../src/queryRunner";
import { VariantAnalysisManager } from "../../../../src/variant-analysis/variant-analysis-manager";
import { QueryHistoryManager } from "../../../../src/query-history/query-history-manager";
import { mockedObject } from "../../utils/mocking.helpers";

// set a higher timeout since recursive delete may take a while, expecially on Windows.
jest.setTimeout(120000);
Expand Down Expand Up @@ -75,14 +75,21 @@ describe("Variant Analyses and QueryHistoryManager", () => {
variantAnalysisManagerStub,
{} as EvalLogViewer,
STORAGE_DIR,
{
mockedObject<ExtensionContext>({
globalStorageUri: Uri.file(STORAGE_DIR),
storageUri: undefined,
extensionPath: EXTENSION_PATH,
} as ExtensionContext,
}),
{
format: "",
ttlInMillis: 0,
onDidChangeConfiguration: () => new DisposableBucket(),
} as unknown as QueryHistoryConfig,
new HistoryItemLabelProvider({} as QueryHistoryConfig),
},
new HistoryItemLabelProvider({
format: "",
ttlInMillis: 0,
onDidChangeConfiguration: jest.fn(),
}),
asyncNoop,
);
disposables.push(qhm);
Expand Down
26 changes: 14 additions & 12 deletions extensions/ql-vscode/test/vscode-tests/no-workspace/test-adapter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,20 @@ describe("test-adapter", () => {
setCurrentDatabaseItemSpy.mockResolvedValue(undefined);
resolveQlpacksSpy.mockResolvedValue({});
resolveTestsSpy.mockResolvedValue([]);
fakeDatabaseManager = {
openDatabase: openDatabaseSpy,
removeDatabaseItem: removeDatabaseItemSpy,
renameDatabaseItem: renameDatabaseItemSpy,
setCurrentDatabaseItem: setCurrentDatabaseItemSpy,
} as unknown as DatabaseManager;
Object.defineProperty(fakeDatabaseManager, "currentDatabaseItem", {
get: () => currentDatabaseItem,
});
Object.defineProperty(fakeDatabaseManager, "databaseItems", {
get: () => databaseItems,
});
fakeDatabaseManager = mockedObject<DatabaseManager>(
{
openDatabase: openDatabaseSpy,
removeDatabaseItem: removeDatabaseItemSpy,
renameDatabaseItem: renameDatabaseItemSpy,
setCurrentDatabaseItem: setCurrentDatabaseItemSpy,
},
{
dynamicProperties: {
currentDatabaseItem: () => currentDatabaseItem,
databaseItems: () => databaseItems,
},
},
);

jest.spyOn(preTestDatabaseItem, "isAffectedByTest").mockResolvedValue(true);

Expand Down
29 changes: 28 additions & 1 deletion extensions/ql-vscode/test/vscode-tests/utils/mocking.helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,39 @@ export type DeepPartial<T> = T extends object
}
: T;

export function mockedObject<T extends object>(props: DeepPartial<T>): T {
export type DynamicProperties<T extends object> = {
[P in keyof T]?: () => T[P];
};

type MockedObjectOptions<T extends object> = {
/**
* Properties for which the given method should be called when accessed.
* The method should return the value to be returned when the property is accessed.
* Methods which are explicitly defined in `methods` will take precedence over
* dynamic properties.
*/
dynamicProperties?: DynamicProperties<T>;
};

export function mockedObject<T extends object>(
props: DeepPartial<T>,
{ dynamicProperties }: MockedObjectOptions<T> = {},
): T {
return new Proxy<T>({} as unknown as T, {
get: (_target, prop) => {
if (prop in props) {
return (props as any)[prop];
}
if (dynamicProperties && prop in dynamicProperties) {
return (dynamicProperties as any)[prop]();
}

// The `then` method is accessed by `Promise.resolve` to check if the object is a thenable.
// We don't want to throw an error when this happens.
if (prop === "then") {
return undefined;
}
Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we undefine then for all mocked objects, then their behaviour might be different than their original counterpart.

Would we be able to check if the original object is thenable?

  • If it is, we could return a mocked method for then (or allow the user to pass in a mocked method? that might be a stretch)
  • If it isn't, return undefined like you've done here.

What do you think?

Copy link
Copy Markdown
Member Author

@koesie10 koesie10 Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice if we could detect whether something is thenable, but unfortunately I don't think we're able to do that. TypeScript doesn't expose the type information at runtime, so we can't get access to the original object. The only way we could do it is by passing in a boolean that matches whether the object is thenable or not.

However, I don't see us mocking a thenable object anytime soon. The only thenable object which is used in our codebase is Promise and this should never be mocked since it can be constructed really easily using Promise.resolve or Promise.reject. I would be in favour of not supporting this usecase and leaving the code as-is, always returning undefined.

Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thenable object which is used in our codebase is Promise and this should never be mocked since it can be constructed really easily using Promise.resolve or Promise.reject.

It looks like there are a few more cases where we use Thenable objects: https://github.com/github/vscode-codeql/search?p=2&q=thenable.

Also other libraries we're using could return Thenable objects. When we try to mock them, we might not be aware of the behaviour change.

However, I don't see us mocking a thenable object anytime soon.

Yeah, I'm more thinking how this isn't a visible thing on mock objects and could therefore affect us without us realising it.

Copy link
Copy Markdown
Member Author

@koesie10 koesie10 Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are a few more cases where we use Thenable objects: https://github.com/github/vscode-codeql/search?p=2&q=thenable.

These are references to the Thenble interface which defines that there must be a then method on some object. It is not an implementation of this interface and in almost all cases it will be a Promise.

Also other libraries we're using could return Thenable objects. When we try to mock them, we might not be aware of the behaviour change.

If we're mocking other libraries, we should still be using mockResolvedValue which will return a Promise. Even other libraries shouldn't implement a thenable object and should be using Promise. I don't see a reason for a library to re-implement a promise, other than for compatibility with older JS versions which didn't support promise. However, even in that case we would still be using mockResolvedValue.

Yeah, I'm more thinking how this isn't a visible thing on mock objects and could therefore affect us without us realising it.

I think the risk here is very low, you'd need to be intentionally mocking a Promise or Thenable using something like mockedObject<Promise<AnotherObject>> or mockedObject<Thenable<AnotherObject>>. This would only work if you also pass an empty object to the mockedObject method since you wouldn't be able to mock any methods of AnotherObject (those will give type errors).

I'm open to suggestions on how we can make this work with promises, but I think it will unnecessarily complicate things because we can't actually detect whether something is thenable and would like to avoid adding another parameter which is always false.

Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure I can suggest a better workaround tbh. Just wanted to understand what the implications are, which you've laid out clearly. 👍

I wasn't familiar with the differences between Thenable and Promise. After reading your explanation, it makes sense that modern libraries would be using Promises.

And we do mock promises intentionally to return what we want, so we have an escape hatch.


throw new Error(`Method ${String(prop)} not mocked`);
},
});
Expand Down