Skip to content

Replace deprecated telemetry setting in the tests #3362

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
shati-patel merged 1 commit into from
Feb 15, 2024
Merged

Replace deprecated telemetry setting in the tests #3362

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 23 additions & 14 deletions extensions/ql-vscode/test/vscode-tests/no-workspace/telemetry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jest.setTimeout(10000);

describe("telemetry reporting", () => {
let originalTelemetryExtension: boolean | undefined;
let originalTelemetryGlobal: boolean | undefined;
let originalTelemetryGlobal: string | undefined;
let isCanary: string;
let ctx: ExtensionContext;
let telemetryListener: ExtensionTelemetryListener;
Expand Down Expand Up @@ -48,7 +48,7 @@ describe("telemetry reporting", () => {
try {
// in case a previous test has accidentally activated this extension,
// need to disable it first.
// Accidentaly activation may happen asynchronously due to activationEvents
// Accidental activation may happen asynchronously due to activationEvents
// specified in the package.json.
globalTelemetryListener?.dispose();

Expand All @@ -73,7 +73,7 @@ describe("telemetry reporting", () => {
.get<boolean>("codeQL.telemetry.enableTelemetry");
originalTelemetryGlobal = workspace
.getConfiguration()
.get<boolean>("telemetry.enableTelemetry");
.get<string>("telemetry.telemetryLevel");
isCanary = (!!workspace
.getConfiguration()
.get<boolean>("codeQL.canary")).toString();
Expand All @@ -82,7 +82,7 @@ describe("telemetry reporting", () => {
isTelemetryEnabledSpy = jest
.spyOn(env, "isTelemetryEnabled", "get")
.mockReturnValue(true);
await enableTelemetry("telemetry", true);
await setTelemetryLevel("telemetry", "all");
await enableTelemetry("codeQL.telemetry", true);

telemetryListener = new ExtensionTelemetryListener(
Expand All @@ -101,27 +101,26 @@ describe("telemetry reporting", () => {
telemetryListener?.dispose();
// await wait(100);
try {
await enableTelemetry("telemetry", originalTelemetryGlobal);
await setTelemetryLevel("telemetry", originalTelemetryGlobal);
await enableTelemetry("codeQL.telemetry", originalTelemetryExtension);
} catch (e) {
console.error(e);
}
});

it("should initialize telemetry when both options are enabled", async () => {
it("should initialize telemetry when 'codeQL.telemetry.enableTelemetry' is enabled and global 'telemetry.telemetryLevel' is 'all'", async () => {
await telemetryListener.initialize();

expect(telemetryListener._reporter).toBeDefined();

const reporter: any = telemetryListener._reporter;
expect(reporter.extensionId).toBe("my-id");
expect(reporter.extensionVersion).toBe("1.2.3");
expect(reporter.userOptIn).toBe(true); // enabled
});

it("should initialize telemetry when global option disabled", async () => {
it("should initialize telemetry when global 'telemetry.telemetryLevel' is 'off'", async () => {
isTelemetryEnabledSpy.mockReturnValue(false);
await enableTelemetry("telemetry", false);
await setTelemetryLevel("telemetry", "off");
await telemetryListener.initialize();
expect(telemetryListener._reporter).toBeDefined();

Expand All @@ -139,7 +138,7 @@ describe("telemetry reporting", () => {
it("should not initialize telemetry when both options disabled", async () => {
await enableTelemetry("codeQL.telemetry", false);
isTelemetryEnabledSpy.mockReturnValue(false);
await enableTelemetry("telemetry", false);
await setTelemetryLevel("telemetry", "off");
await telemetryListener.initialize();
expect(telemetryListener._reporter).toBeUndefined();
});
Expand Down Expand Up @@ -179,14 +178,14 @@ describe("telemetry reporting", () => {
expect(disposeSpy).toHaveBeenCalledTimes(1);
});

it("should set userOprIn to false when global setting changes", async () => {
it("should set userOptIn to false when global setting changes", async () => {
await telemetryListener.initialize();

const reporter: any = telemetryListener._reporter;
expect(reporter.userOptIn).toBe(true); // enabled

isTelemetryEnabledSpy.mockReturnValue(false);
await enableTelemetry("telemetry", false);
await setTelemetryLevel("telemetry", "off");
expect(reporter.userOptIn).toBe(false); // disabled
});

Expand Down Expand Up @@ -420,7 +419,7 @@ describe("telemetry reporting", () => {
// show the dialog.

isTelemetryEnabledSpy.mockReturnValue(false);
await enableTelemetry("telemetry", false);
await setTelemetryLevel("telemetry", "off");
await ctx.globalState.update("telemetry-request-viewed", false);

await telemetryListener.initialize();
Expand All @@ -431,7 +430,7 @@ describe("telemetry reporting", () => {

// This test is failing because codeQL.canary is not a registered configuration.
// We do not want to have it registered because we don't want this item
// appearing in the settings page. It needs to olny be set by users we tell
// appearing in the settings page. It needs to only be set by users we tell
// about it to.
// At this point, I see no other way of testing re-requesting permission.
xit("should request permission again when user changes canary setting", async () => {
Expand Down Expand Up @@ -574,6 +573,16 @@ describe("telemetry reporting", () => {
await wait(100);
}

async function setTelemetryLevel(section: string, value: string | undefined) {
await workspace
.getConfiguration(section)
.update("telemetryLevel", value, ConfigurationTarget.Global);

// Need to wait some time since the onDidChangeConfiguration listeners fire
// asynchronously. Must ensure they to complete in order to have a successful test.
await wait(100);
Copy link
Copy Markdown
Contributor

@charisk charisk Feb 15, 2024

Choose a reason for hiding this comment

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

An observation, but not introduced in this PR so doesn't need to be address now: We seem to call enableTelemetry and setTelemetry sequentially. They're both updating the config and then wait for 100ms. I wonder if we could merge the calls into one and only wait once. It would shave off some waiting timing.

Copy link
Copy Markdown
Contributor Author

@shati-patel shati-patel Feb 15, 2024

Choose a reason for hiding this comment

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

Good point! 👀 I'll leave things as-is for now, since there'll likely be more changes to the tests anyway, if we manage to upgrade the telemetry package 🤞🏽

}

async function wait(ms = 0) {
return new Promise((resolve) => setTimeout(resolve, ms));
}
Expand Down