This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7961-9f2102e9b0d83b7b518f37bab3ae497d3a0e9b20 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 7d5aea83b1d9360be8214fbd04923204e2fbac6f Author: Meng Wang <[email protected]> AuthorDate: Tue Aug 25 22:09:58 2026 +0000 test(frontend): cover the admin settings uploads and save guards (#7961) ### What changes were proposed in this PR? Extends `admin-settings.component.spec.ts` with 9 tests covering the last uncovered lines and half-taken branches. Measured locally with `--coverage --coverage-reporters=lcovonly`: | `admin-settings.component.ts` | Before | After | | --- | --- | --- | | lines | 104/112 (92.86 %) | **112/112 (100 %)** | | branches | 46/54 | **54/54** | Every line and branch the issue lists is now covered. - **`onFileChange`** — the spec already had a `FileReader` double for one arm; it is lifted into a helper so the logo arm, the favicon arm and the type guard share it. A result that is not a string (an `ArrayBuffer`) takes the other side of `typeof e.target?.result === "string" ? … : null` and clears the field. - **`saveLogos`** — the favicon leg, asserting all three PUTs are issued and that the favicon request carries the right body (the existing test covers the case where it is absent). - **`partsAtMax` / `requiredMinPartSizeMiB`** — each missing-value combination, so both early returns are taken, plus one case above the floor where the 10,000-part limit decides the result. - **`saveDatasetSettings` / `saveCsvSettings`** — the `settingsLoaded` guard: the `ngOnInit` bulk GET is deliberately left outstanding so the flag is still false, then the test asserts the error message and that no PUT is issued. - **`resetCsvSettings`** — the error handler, by failing the reset request. Two places where the issue's description and the code disagree; the tests follow the code and the difference is worth flagging for the next reader: 1. The issue says `onFileChange` stores into `logoData` for `"logo"` or `faviconData` for `"mini_logo"`. The component actually routes `mini_logo` to `miniLogoData` and the remaining type (`favicon`) to `faviconData` via the `else`, which is what lines 149 and 153 are. 2. The issue attributes lines 227/232 to `resetTabs`. Those lines are the `partsAtMax` and `requiredMinPartSizeMiB` getters that sit just below it, so the tests target the getters. No production code was changed. ### Any related issues, documentation, discussions? Closes #7913. ### How was this PR tested? `ng test --watch=false --include src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts` — 45 passed (36 before, 9 new), repeated 3× for stability; the whole `dashboard/component/admin/**` folder stays green at 156 passed. `yarn format:ci` clean. Failure path verified by breaking one assertion in each of the 9 new tests: 9 failed / 36 passed, non-zero exit, then restored to green. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../settings/admin-settings.component.spec.ts | 140 +++++++++++++++++++-- 1 file changed, 130 insertions(+), 10 deletions(-) diff --git a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts index e33b2bb7f3..4c764b99fd 100644 --- a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts +++ b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts @@ -188,6 +188,25 @@ describe("AdminSettingsComponent", () => { expect(msgSuccess).toHaveBeenCalledWith("Branding saved successfully."); }); + it("saveLogos PUTs the favicon too when one is set", () => { + completeLoad(); + component.logoData = "logo.png"; + component.miniLogoData = "mini.png"; + component.faviconData = "fav.ico"; + + component.saveLogos(); + + const requests = ["logo", "mini_logo", "favicon"].map(key => { + const req = httpTestingController.expectOne(updateUrl(key)); + expect(req.request.method).toBe("PUT"); + return req; + }); + expect(requests[2].request.body).toEqual({ value: "fav.ico" }); + requests.forEach(req => req.flush(null)); + + expect(msgSuccess).toHaveBeenCalledWith("Branding saved successfully."); + }); + it("saveLogos does nothing when no branding asset is set", () => { completeLoad(); component.logoData = null; @@ -288,6 +307,17 @@ describe("AdminSettingsComponent", () => { expect(msgSuccess).toHaveBeenCalledWith("Dataset upload settings saved successfully."); }); + it("saveDatasetSettings refuses to save before the bulk load completes", () => { + // The ngOnInit GET is left outstanding on purpose: settingsLoaded is still false. + const pending = httpTestingController.expectOne(SETTINGS_URL); + + component.saveDatasetSettings(); + + httpTestingController.expectNone((req: { method: string }) => req.method === "PUT"); + expect(msgError).toHaveBeenCalledWith("Settings have not loaded; refresh before saving."); + pending.flush({}); + }); + it("saveDatasetSettings rejects non-positive values without saving", () => { completeLoad(); component.maxFileSizeMiB = 0; @@ -344,6 +374,37 @@ describe("AdminSettingsComponent", () => { }); }); + // The issue labels these lines as `resetTabs`; they are actually the two computed + // getters that sit just below it, so the tests target the getters. + describe("computed part-size properties", () => { + it("partsAtMax is 0 unless both the total size and the chunk size are set", () => { + completeLoad(); + + component.maxFileSizeMiB = 0; + component.chunkSizeMiB = 8; + expect(component.partsAtMax).toBe(0); + + component.maxFileSizeMiB = 100; + component.chunkSizeMiB = 0; + expect(component.partsAtMax).toBe(0); + + component.maxFileSizeMiB = 100; + component.chunkSizeMiB = 8; + expect(component.partsAtMax).toBe(13); + }); + + it("requiredMinPartSizeMiB falls back to the floor when no total size is set", () => { + completeLoad(); + + component.maxFileSizeMiB = 0; + expect(component.requiredMinPartSizeMiB).toBe(component.MIN_PART_SIZE_MiB); + + // Above the floor the parts limit takes over: 10,000 parts must cover the total. + component.maxFileSizeMiB = component.MIN_PART_SIZE_MiB * component.MAX_TOTAL_PARTS * 2; + expect(component.requiredMinPartSizeMiB).toBe(component.MIN_PART_SIZE_MiB * 2); + }); + }); + describe("csv (result panel) settings", () => { it("saveCsvSettings PUTs the max-columns value and notifies success", () => { completeLoad(); @@ -359,6 +420,16 @@ describe("AdminSettingsComponent", () => { expect(notifySuccess).toHaveBeenCalledWith("Result panel settings saved."); }); + it("saveCsvSettings refuses to save before the bulk load completes", () => { + const pending = httpTestingController.expectOne(SETTINGS_URL); + + component.saveCsvSettings(); + + httpTestingController.expectNone((req: { method: string }) => req.method === "PUT"); + expect(msgError).toHaveBeenCalledWith("Settings have not loaded; refresh before saving."); + pending.flush({}); + }); + it("saveCsvSettings notifies an error when the request fails", () => { completeLoad(); @@ -368,6 +439,15 @@ describe("AdminSettingsComponent", () => { expect(notifyError).toHaveBeenCalledWith("Could not save result panel settings."); }); + it("resetCsvSettings notifies an error when the reset fails", () => { + completeLoad(); + + component.resetCsvSettings(); + + httpTestingController.expectOne(resetUrl("csv_parser_max_columns")).flush("boom", HTTP_ERROR); + expect(notifyError).toHaveBeenCalledWith("Could not reset result panel settings."); + }); + it("resetCsvSettings POSTs a reset and notifies info", () => { completeLoad(); @@ -392,30 +472,70 @@ describe("AdminSettingsComponent", () => { expect(component.logoData).toBe("data:image/png;base64,EXISTING"); }); - it("reads a valid image file into the matching branding field", async () => { - completeLoad(); - const dataUrl = "data:image/png;base64,AAA"; + const dataUrl = "data:image/png;base64,AAA"; + + /** + * Uploads a valid image through a FileReader double that resolves to `result`. + * The double keeps the read off jsdom's real async, so the assertion only has to + * wait for the microtask the fake itself queues. + */ + async function uploadImageResolvingTo(type: "logo" | "mini_logo" | "favicon", result: unknown): Promise<void> { class FakeFileReader { - onload: ((e: { target: { result: string } }) => void) | null = null; + onload: ((e: { target: { result: unknown } }) => void) | null = null; readAsDataURL(): void { - queueMicrotask(() => this.onload?.({ target: { result: dataUrl } })); + queueMicrotask(() => this.onload?.({ target: { result } })); } } const realFileReader = globalThis.FileReader; (globalThis as any).FileReader = FakeFileReader; - try { const event = { target: { files: [new File(["x"], "logo.png", { type: "image/png" })] }, } as unknown as Event; - - component.onFileChange("mini_logo", event); + component.onFileChange(type, event); await Promise.resolve(); - - expect(component.miniLogoData).toBe(dataUrl); } finally { (globalThis as any).FileReader = realFileReader; } + } + + it("reads a valid image file into the matching branding field", async () => { + completeLoad(); + + await uploadImageResolvingTo("mini_logo", dataUrl); + + expect(component.miniLogoData).toBe(dataUrl); + }); + + it("routes a logo upload to logoData", async () => { + completeLoad(); + + await uploadImageResolvingTo("logo", dataUrl); + + expect(component.logoData).toBe(dataUrl); + expect(component.miniLogoData).toBeNull(); + expect(component.faviconData).toBeNull(); + }); + + it("routes a favicon upload to faviconData", async () => { + completeLoad(); + + await uploadImageResolvingTo("favicon", dataUrl); + + expect(component.faviconData).toBe(dataUrl); + expect(component.logoData).toBeNull(); + expect(component.miniLogoData).toBeNull(); + }); + + it("stores null when the reader yields something other than a string", async () => { + completeLoad(); + component.logoData = "data:image/png;base64,EXISTING"; + + // readAsDataURL always yields a string, but the handler guards the type anyway; + // an ArrayBuffer result takes the other arm of that ternary. + await uploadImageResolvingTo("logo", new ArrayBuffer(8)); + + expect(component.logoData).toBeNull(); }); }); });
