Skip to content
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.

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

Only clear mod component debug logs if logValues is enabled #9225

Merged
merged 2 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 44 additions & 10 deletions src/contentScript/contentScriptPlatform.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,29 +24,39 @@ import { InteractiveLoginRequiredError } from "@/errors/authErrors";
import { waitForEffect } from "@/testUtils/testHelpers";
import { deferLogin } from "@/contentScript/integrations/deferredLoginController";
import pDefer from "p-defer";
import { performConfiguredRequestInBackground } from "@/background/messenger/api";
import {
clearModComponentDebugLogs,
performConfiguredRequestInBackground,
traces,
} from "@/background/messenger/api";
import { API_PATHS } from "@/data/service/urlPaths";
import { modComponentFactory } from "@/testUtils/factories/modComponentFactories";

jest.mock("@/contentScript/integrations/deferredLoginController");

jest.mock("@/background/messenger/api", () => ({
performConfiguredRequestInBackground: jest.fn().mockResolvedValue({}),
clearModComponentDebugLogs: jest.fn(),
traces: {
clear: jest.fn(),
},
}));

const deferLoginMock = jest.mocked(deferLogin);

const backgroundRequestMock = jest.mocked(performConfiguredRequestInBackground);
const clearModComponentDebugLogsMock = jest.mocked(clearModComponentDebugLogs);
const tracesClearMock = jest.mocked(traces.clear);

beforeEach(() => {
setPlatform(contentScriptPlatform);
});
describe("contentScriptPlatform", () => {
beforeEach(() => {
setPlatform(contentScriptPlatform);
});

afterEach(async () => {
jest.clearAllMocks();
await TEST_deleteFeatureFlagsCache();
});
afterEach(async () => {
jest.clearAllMocks();
await TEST_deleteFeatureFlagsCache();
});

describe("contentScriptPlatform", () => {
it("makes non-interactive successful call", async () => {
appApiMock.onGet(API_PATHS.FEATURE_FLAGS).reply(200, { flags: [] });

Expand Down Expand Up @@ -119,4 +129,28 @@ describe("contentScriptPlatform", () => {

await expect(requestPromise).resolves.toBeObject();
});

describe("debugger", () => {
it("clears traces and clears logs when logValues is true", async () => {
const componentId = modComponentFactory().id;
await contentScriptPlatform.debugger.clear(componentId, {
logValues: true,
});

expect(tracesClearMock).toHaveBeenCalledExactlyOnceWith(componentId);
expect(clearModComponentDebugLogsMock).toHaveBeenCalledExactlyOnceWith(
componentId,
);
});

it("clears traces and skips clearing logs when logValues is false", async () => {
const componentId = modComponentFactory().id;
await contentScriptPlatform.debugger.clear(componentId, {
logValues: false,
});

expect(tracesClearMock).toHaveBeenCalledExactlyOnceWith(componentId);
expect(clearModComponentDebugLogsMock).not.toHaveBeenCalled();
});
});
});
16 changes: 11 additions & 5 deletions src/contentScript/contentScriptPlatform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,17 @@ class ContentScriptPlatform extends PlatformBase {

override get debugger(): PlatformProtocol["debugger"] {
return {
async clear(componentId: UUID): Promise<void> {
await Promise.all([
traces.clear(componentId),
clearModComponentDebugLogs(componentId),
]);
async clear(
componentId: UUID,
{ logValues }: { logValues: boolean },
): Promise<void> {
const clearPromises = [traces.clear(componentId)];

if (logValues) {
clearPromises.push(clearModComponentDebugLogs(componentId));
}

await Promise.all(clearPromises);
},
traces: {
enter: traces.addEntry,
Expand Down
65 changes: 65 additions & 0 deletions src/extensionPages/extensionPagePlatform.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright (C) 2024 PixieBrix, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import extensionPagePlatform from "@/extensionPages/extensionPagePlatform";
import { setPlatform } from "@/platform/platformContext";
import { modComponentFactory } from "@/testUtils/factories/modComponentFactories";
import { clearModComponentDebugLogs, traces } from "@/background/messenger/api";

jest.mock("@/background/messenger/api", () => ({
clearModComponentDebugLogs: jest.fn(),
traces: {
clear: jest.fn(),
},
}));

const tracesClearMock = jest.mocked(traces.clear);
const clearModComponentDebugLogsMock = jest.mocked(clearModComponentDebugLogs);

describe("extensionPagePlatform", () => {
beforeEach(() => {
setPlatform(extensionPagePlatform);
});

afterEach(() => {
jest.clearAllMocks();
});

describe("debugger", () => {
it("clears traces and clears logs when logValues is true", async () => {
const componentId = modComponentFactory().id;
await extensionPagePlatform.debugger.clear(componentId, {
logValues: true,
});

expect(tracesClearMock).toHaveBeenCalledExactlyOnceWith(componentId);
expect(clearModComponentDebugLogsMock).toHaveBeenCalledExactlyOnceWith(
componentId,
);
});

it("clears traces and skips clearing logs when logValues is false", async () => {
const componentId = modComponentFactory().id;
await extensionPagePlatform.debugger.clear(componentId, {
logValues: false,
});

expect(tracesClearMock).toHaveBeenCalledExactlyOnceWith(componentId);
expect(clearModComponentDebugLogsMock).not.toHaveBeenCalled();
});
});
});
16 changes: 11 additions & 5 deletions src/extensionPages/extensionPagePlatform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,17 @@ class ExtensionPagePlatform extends PlatformBase {
// Support tracing for bricks run in the sidebar and clearing logs in Page Editor/Extension Console. See PanelBody.tsx
override get debugger(): PlatformProtocol["debugger"] {
return {
async clear(componentId: UUID): Promise<void> {
await Promise.all([
traces.clear(componentId),
clearModComponentDebugLogs(componentId),
]);
async clear(
componentId: UUID,
{ logValues }: { logValues: boolean },
): Promise<void> {
const clearPromises = [traces.clear(componentId)];

if (logValues) {
clearPromises.push(clearModComponentDebugLogs(componentId));
}

await Promise.all(clearPromises);
},
traces: {
enter: traces.addEntry,
Expand Down
5 changes: 4 additions & 1 deletion src/platform/platformTypes/debuggerProtocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ export interface DebuggerProtocol {
*
* @param componentId the mod component id
*/
clear: (componentId: UUID) => Promise<void>;
clear: (
componentId: UUID,
{ logValues }: { logValues: boolean },
Copy link
Collaborator

@fungairino fungairino Oct 3, 2024

Choose a reason for hiding this comment

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

We could consider renaming this option to something like clearDebugLogs for clarity

Another consideration is whether to expose this in the protocol interface, or just get the logValues value in the underlying implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per our sync discussion, we're going to keep it as logValues for consistency. And the implementation doesn't have access to the source for logValues, so we're leaving that to reducePipeline

) => Promise<void>;

traces: TraceProtocol;
}
16 changes: 11 additions & 5 deletions src/runtime/reducePipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,14 @@ type RunBrickOptions = CommonOptions & {
trace: TraceMetadata;
};

async function getLogValues(
logValues: RunBrickOptions["logValues"] | undefined,
): Promise<boolean> {
const globalLoggingConfig = await getLoggingConfig();

return logValues ?? globalLoggingConfig.logValues ?? false;
}

/**
* Get the lexical environment for running a pipeline. Currently, we're just tracking on the pipeline arg itself.
* https://en.wikipedia.org/wiki/Closure_(computer_programming)
Expand Down Expand Up @@ -456,11 +464,9 @@ async function renderBrickArg(
): Promise<RenderedArgs> {
const { config, type } = resolvedConfig;

const globalLoggingConfig = await getLoggingConfig();

const {
// If logValues not provided explicitly, default to the global setting
logValues = globalLoggingConfig.logValues ?? false,
logValues = await getLogValues(options.logValues),
logger,
explicitArg,
explicitDataFlow,
Expand Down Expand Up @@ -637,7 +643,6 @@ async function applyReduceDefaults({
Partial<ReduceOptions>,
"modComponentRef"
>): Promise<ReduceOptions> {
const globalLoggingConfig = await getLoggingConfig();
const logger = providedLogger ?? new ConsoleLogger();

return {
Expand All @@ -652,7 +657,7 @@ async function applyReduceDefaults({
explicitDataFlow: false,
extendModVariable: false,
// If logValues not provided explicitly, default to the global setting
logValues: logValues ?? globalLoggingConfig.logValues ?? false,
logValues: await getLogValues(logValues),
// For stylistic consistency, default here instead of destructured parameters
branches: [],
// NOTE: do not set runId here. It should be set by the starter brick explicitly, or implicitly generated
Expand Down Expand Up @@ -960,6 +965,7 @@ export async function reduceModComponentPipeline(
// `await` promise to avoid race condition where the calls here delete entries from this call to reducePipeline
await platform.debugger.clear(
partialOptions.modComponentRef.modComponentId,
{ logValues: await getLogValues(partialOptions.logValues) },
);
} catch {
// NOP
Expand Down
Loading