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-8336-464aa8c19e10cedcc727c913443619603b90157d in repository https://gitbox.apache.org/repos/asf/texera.git
commit 30681cd38406ca273f94714da2d7b118d8100f9f Author: Xinyuan Lin <[email protected]> AuthorDate: Wed Sep 2 11:01:06 2026 +0000 test(frontend): close the user, hub and resource-registry service gaps (#8336) ### What changes were proposed in this PR? Four existing frontend specs extended. **+10 fully-covered lines and +7 branch arms.** | File | Codecov | Branch arms | |---|---|---| | `user.service.ts` | 62/66 → **66/66** | 19/21 → **21/21** | | `browse-section.component.ts` | 28/32 → 31/32 | 22/25 → 24/25 | | `resource-registry.service.ts` | 18/20 → **20/20** | 16/17 → **17/17** | | `email-request-modal.component.html` | 14/15 → **15/15** | 4/6 → **6/6** | Three of the four reach 100%. The plain line-hit metric moves only **+4** against Codecov's +10, because six of the gained lines were already executing and flip solely by completing a branch arm — the two numbers are not interchangeable and both are given. Not-fully-covered across the bundle goes 13 → 3, and all three of those are deliberately declined (below). ### The reviewer revised my own claim down The build reported +11 lines and +8 arms. Independent re-measurement puts it at **+10 and +7**: `browse-section.component.ts` gains 3 lines and 2 arms, not 4 and 3. Line 96 remains partial. The lower figure is the one in the table. ### Two files were in scope and contribute nothing - **`user-dataset-version-creator.component.ts` was dropped entirely.** Its only missed line is `get formControlNames()`, and a repo-wide grep across `.ts` and `.html` returns exactly one hit — its own declaration. **Zero call sites, zero template bindings.** A test there would be a pure count-raiser, so the getter is flagged as a dead-code removal candidate instead. - `browse-section.component.html` (24/24) and `email-request-modal.component.ts` (8/9) were measured both ways and are unchanged. ### Deliberately not included `browse-section.component.ts:96` stays partial, and `email-request-modal.component.ts:67` is declined. One survivor is reported rather than chased: mutating `user.service.ts:108` from `handleAccessToken(accessToken ?? "")` to a conditional survives the suite. That mutant is character-for-character the shape `register()` already uses for the same `string | null` payload — it is arguably the *fix*, not a regression, so no test was written to pin the current form. `user.service.ts:184` (`this.cache.delete(avatarUrl)`) is covered but **not independently pinned**: the code immediately falls through to a `map` that overwrites the entry either way, so no mutation isolates it. It rides along with lines 180 and 183 in one test, and that is stated rather than presented as a kill. ### Verification Measured with the **full 209-file suite in one command** — no name filter and no `--include` at all — so there is no filter-attribution risk, with `coverage/` deleted before each run. The baseline was rebuilt by writing the `HEAD` versions of the four specs into place from a scratch extraction (never `git checkout`), running, then restoring from a hash-verified snapshot. Figures parsed straight from `coverage/gui/lcov.info`. Seven reviewer findings, all repaired — including one test that was **deleted** rather than kept, because it duplicated an existing assertion. `browse-section.component.ts:114` needed a white-box assertion or the test would have been vacuous: `getCoverImage()`'s `|| this.defaultBackground` makes the guarded and unguarded paths observably identical through the public API, so the test asserts on the private `coverImageUrls` map. That is unusual and is called out here rather than left for a reader to find. `yarn format:ci` passes. `frontend/junit.xml` is regenerated by every run, is not gitignored, and is not committed. No production file is touched. ### Any related issues, documentation, discussions? Closes #8334 ### How was this PR tested? ``` npx ng test --watch=false --include="**/user.service.spec.ts" --include="**/browse-section.component.spec.ts" --include="**/resource-registry.service.spec.ts" --include="**/email-request-modal.component.spec.ts" ``` ``` Test Files 4 passed (4) ``` Re-run after rebasing onto current `main`. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> --- .../email-request-modal.component.spec.ts | 29 ++++++++ .../app/common/service/user/user.service.spec.ts | 83 +++++++++++++++++++++- .../resource-registry.service.spec.ts | 38 ++++++++++ .../browse-section.component.spec.ts | 61 +++++++++++++++- 4 files changed, 208 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts index a00bd7a60f..93e25de2ed 100644 --- a/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts +++ b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts @@ -140,6 +140,35 @@ describe("EmailRequestModalComponent", () => { expect(code.getAttribute("autocomplete")).toBe("one-time-code"); }); + /** + * The spec above reads the code box's rendered attributes; nothing had ever typed into it, so its + * [(ngModel)] write-back had never fired. That is the direction that matters: the dialog's caller + * reads the code out of getValues(), and a one-way binding here would hand it a blank code with + * no visible symptom on screen. + */ + it("writes the typed verification code back through ngModel", async () => { + const fixture = await createFixture({ name: "Sofia" }); + fixture.detectChanges(); + fixture.componentInstance.email = "[email protected]"; + fixture.componentInstance.step = "code"; + fixture.detectChanges(); + + const [, code] = inputs(fixture); + code.value = "246810"; + code.dispatchEvent(new Event("input")); + fixture.detectChanges(); + + expect(fixture.componentInstance.code).toBe("246810"); + // The two boxes are not crossed: the frozen address is still the one it was mailed to. + expect(fixture.componentInstance.getValues()).toEqual({ email: "[email protected]", code: "246810" }); + // ...and not only in the fields. This is the one test that renders the code step with a + // non-empty code, so it is the only place the label naming where the code went can be told + // apart from the code itself; through getValues() the two are indistinguishable on screen. + const label = (fixture.nativeElement as HTMLElement).querySelector(".email-modal-code-label"); + expect(label?.textContent).toContain("[email protected]"); + expect(label?.textContent).not.toContain("246810"); + }); + function inputs(fixture: ComponentFixture<EmailRequestModalComponent>): HTMLInputElement[] { return Array.from(fixture.nativeElement.querySelectorAll("input")); } diff --git a/frontend/src/app/common/service/user/user.service.spec.ts b/frontend/src/app/common/service/user/user.service.spec.ts index 415335138c..6a38246626 100644 --- a/frontend/src/app/common/service/user/user.service.spec.ts +++ b/frontend/src/app/common/service/user/user.service.spec.ts @@ -106,6 +106,34 @@ describe("UserService", () => { expect(service.isLogin()).toBe(true); }); + // The verify endpoint answers with `accessToken: string | null`, so a backend that accepted the + // code but declined to mint a session is representable. registerVerify substitutes "" for the + // absent token. What is pinned here is narrow and deliberate: no *usable* token is left behind, + // so any substituted placeholder fails. + // + // What is deliberately NOT pinned is whether the absent-token path runs `handleAccessToken` at + // all. It does today, and that is the defect: it writes "" over whatever token was there and + // fires the post-login @RolesAllowed config fetch for a session that was never granted. With a + // token already in localStorage that path reaches `AuthService.loginWithExistingToken`, whose + // `token == null` guard does not catch "", so an unrelated failed verification pops "Access + // token is expired!" and signs the current user out. `register()` above handles the same + // `string | null` shape by short-circuiting instead. Asserting either symptom — the "" write, or + // the config fetch firing — would cement that, so this test stays silent about it and accepts + // that a short-circuiting rewrite of line 108 is not distinguishable from here. + it("leaves no usable session when registerVerify answers without a token", async () => { + const auth = TestBed.inject(AuthService) as unknown as StubAuthService; + vi.spyOn(auth, "registerVerify").mockReturnValue(of({ accessToken: null })); + + await firstValueFrom(service.registerVerify("pending", "[email protected]", "password", "123456")); + + // Tolerant of both "" (today) and null (a short-circuiting fix), intolerant of any substituted + // placeholder. `isLogin()` is deliberately not asserted next to it: the stub's + // loginWithExistingToken answers with a user only for MOCK_TOKEN, which no mutation of this + // path can synthesise out of `{ accessToken: null }`, so that assertion could never have + // failed and read as assurance it did not provide. + expect(AuthService.getAccessToken() ?? "").toBe(""); + }); + it("should not login after login failed", () => { expect((service as any).currentUser).toBeFalsy(); service @@ -260,15 +288,18 @@ describe("UserService", () => { // so stub them deterministically and restore the originals afterwards. let originalFetch: typeof globalThis.fetch; let originalCreateObjectURL: typeof URL.createObjectURL; + let originalRevokeObjectURL: typeof URL.revokeObjectURL; beforeEach(() => { originalFetch = globalThis.fetch; originalCreateObjectURL = URL.createObjectURL; + originalRevokeObjectURL = URL.revokeObjectURL; }); afterEach(() => { globalThis.fetch = originalFetch; URL.createObjectURL = originalCreateObjectURL; + URL.revokeObjectURL = originalRevokeObjectURL; }); it("fetches the avatar, wraps the blob in an object URL, and caches it", async () => { @@ -276,14 +307,23 @@ describe("UserService", () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, blob: () => Promise.resolve(blob) }) as any; URL.createObjectURL = vi.fn().mockReturnValue("blob:fetched"); - const result = await firstValueFrom(service.getAvatar(AVATAR_URL)); + // Held rather than subscribed inline, and subscribed twice: the returned observable replays + // its one value to every later subscriber. Under a refCounted share the first subscriber's + // unsubscribe would tear the buffer down, so the second would refetch and mint a second + // object URL over the cache entry — orphaning the first blob with nothing left holding the + // reference needed to revoke it. Same leak class as the expired-entry revoke below. + const avatar$ = service.getAvatar(AVATAR_URL); + + expect(await firstValueFrom(avatar$)).toBe("blob:fetched"); + expect(await firstValueFrom(avatar$)).toBe("blob:fetched"); - expect(result).toBe("blob:fetched"); // fetched verbatim — no CDN prefix is reconstructed here any more expect(globalThis.fetch).toHaveBeenCalledWith(AVATAR_URL, { referrerPolicy: "no-referrer", }); expect(URL.createObjectURL).toHaveBeenCalledWith(blob); + expect(globalThis.fetch).toHaveBeenCalledTimes(1); + expect(URL.createObjectURL).toHaveBeenCalledTimes(1); }); it("fetches an avatar hosted anywhere the backend allowed, not just Google's CDN", async () => { @@ -296,6 +336,45 @@ describe("UserService", () => { expect(globalThis.fetch).toHaveBeenCalledWith(otherHost, { referrerPolicy: "no-referrer" }); }); + // The freshness check has an expiry side to it: an object URL that outlives its entry has to be + // handed back to the browser, or every avatar refresh leaks the blob it was holding. The + // "returns the cached object URL while the entry is still fresh" spec above only ever exercised + // the other side of the same comparison. + it("revokes the stale object URL and refetches once the cached entry has expired", async () => { + const blob = new Blob(["fresh-img"]); + globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, blob: () => Promise.resolve(blob) }) as any; + URL.createObjectURL = vi.fn().mockReturnValue("blob:fresh"); + URL.revokeObjectURL = vi.fn(); + (service as any).cache.set(AVATAR_URL, { url: "blob:stale", expiry: Date.now() - 1 }); + + const result = await firstValueFrom(service.getAvatar(AVATAR_URL)); + + // Released by its own url, not by the cache key it was filed under. + expect(URL.revokeObjectURL).toHaveBeenCalledWith("blob:stale"); + expect(globalThis.fetch).toHaveBeenCalledWith(AVATAR_URL, { referrerPolicy: "no-referrer" }); + expect(result).toBe("blob:fresh"); + // The whole entry, not just its url. Both freshness tests hand the comparison an expiry the + // test itself supplied, so without this nothing anywhere reads back the expiry getAvatar + // *writes*: `Date.now() + cacheDuration` could read `- cacheDuration` and every entry would + // be born stale, turning every avatar render into a revoke-and-refetch. + const entry = (service as any).cache.get(AVATAR_URL); + expect(entry.url).toBe("blob:fresh"); + expect(entry.expiry).toBeGreaterThan(Date.now()); + }); + + it("drops the revoked entry from the cache even when the refetch then fails", async () => { + URL.revokeObjectURL = vi.fn(); + globalThis.fetch = vi.fn().mockResolvedValue({ ok: false, status: 500 }) as any; + (service as any).cache.set(AVATAR_URL, { url: "blob:stale", expiry: Date.now() - 1 }); + + expect(await firstValueFrom(service.getAvatar(AVATAR_URL))).toBeUndefined(); + + // The url has been handed back to the browser, so the entry holding it is dead. Keeping it + // would leave a revoked object URL reachable and revoke it a second time on the next call. + expect((service as any).cache.has(AVATAR_URL)).toBe(false); + expect(URL.revokeObjectURL).toHaveBeenCalledTimes(1); + }); + it("returns undefined when the avatar fetch fails", async () => { globalThis.fetch = vi.fn().mockResolvedValue({ ok: false, status: 500 }) as any; expect(await firstValueFrom(service.getAvatar("https://lh3.googleusercontent.com/a/BAD";))).toBeUndefined(); diff --git a/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts b/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts index 65efc56709..0b15d9be65 100644 --- a/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts +++ b/frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts @@ -27,6 +27,7 @@ import { DatasetService } from "../dataset/dataset.service"; import { ModelService } from "../model/model.service"; import { WorkflowPersistService } from "../../../../common/service/workflow-persist/workflow-persist.service"; import { DownloadService } from "../download/download.service"; +import { FileResourceDescriptor } from "./file-resource.descriptor"; import { HUB_DATASET_RESULT_DETAIL, HUB_MODEL_RESULT_DETAIL, @@ -247,4 +248,41 @@ describe("ResourceRegistryService", () => { expect(registry.entryLink(entry({ type: EntityType.Dataset, id: undefined }), 42)).toEqual([]); expect(registry.entryLink(entry({ type: EntityType.Workflow, id: "draft" }), 42)).toEqual([]); }); + + /** + * `hubRoute` is optional on the descriptor contract, and the shipped kinds happen to declare + * both routes or neither — so nothing had ever asked what a private-page-only kind links to. + * The answer must not depend on the viewer: with nowhere else to send them, the private page is + * the only link there is, and the access check further down would otherwise route an outsider to + * `undefined`. Descriptors reach the registry by injection, so the kind is supplied as one. + */ + it("links a kind with a private page and no hub page straight to its private page", () => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [ + { provide: DownloadService, useValue: downloadService }, + { provide: WorkflowPersistService, useValue: workflowPersistService }, + { provide: DatasetService, useValue: datasetService }, + { provide: ModelService, useValue: modelService }, + { + provide: FileResourceDescriptor, + useValue: { + type: EntityType.File, + iconType: "folder-open", + privateRoute: "/private-files", + isOwner: () => true, + }, + }, + ...commonTestProviders, + ], + }); + const privatePageOnly = TestBed.inject(ResourceRegistryService); + const file = entry({ type: EntityType.File, id: 7, accessibleUserIds: [42] }); + + expect(privatePageOnly.entryLink(file, 42)).toEqual(["/private-files", "7"]); + // Same link for a viewer with no access, and for an anonymous one. + expect(privatePageOnly.entryLink(file, 99)).toEqual(["/private-files", "7"]); + expect(privatePageOnly.entryLink(file, undefined)).toEqual(["/private-files", "7"]); + }); }); diff --git a/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts b/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts index 07feb44e51..8531fb5247 100644 --- a/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts +++ b/frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts @@ -128,7 +128,12 @@ describe("BrowseSectionComponent", () => { }); describe("cover images", () => { - it("caches the cover URL the descriptor resolves for an entity that has a cover", () => { + it("caches the cover URL the descriptor resolves for an entity that has a cover, and asks once", () => { + // Spied, not replaced, so the double still answers: the call count is what pins the + // `!coverImageUrls.has(cacheKey(entity))` filter. ngOnChanges runs on every input change of + // every one of the landing page's four sections, so without that filter each pass would + // re-resolve every cover — a fresh presigned-URL request per card per change-detection run. + const cover = vi.spyOn(TestBed.inject(DatasetService) as any, "getDatasetCoverUrl"); const entity = { id: 5, type: "dataset", @@ -137,8 +142,10 @@ describe("BrowseSectionComponent", () => { } as unknown as DashboardEntry; component.entities = [entity]; component.ngOnInit(); + component.ngOnChanges({} as any); expect(component.getCoverImage(entity)).toBe(PRESIGNED_COVER); + expect(cover).toHaveBeenCalledTimes(1); }); it("falls back to the default background when no cover was cached", () => { @@ -149,6 +156,58 @@ describe("BrowseSectionComponent", () => { expect(component.getCoverImage(entity)).toBe(component.defaultBackground); }); + + // `this.resourceRegistry.find(entity.type)?.coverUrl` carries two guards, and a mixed section + // can trip either. A workflow's cover is a data URL carried on the entry itself, so + // WorkflowResourceDescriptor deliberately declares no `coverUrl`; and a kind the registry does + // not carry at all has no descriptor to ask, which is why this is `find`, not `get` — one such + // row must not take the whole section's covers down, exactly as `routeFor` five lines up + // already promises for links. + it("skips an entity whose descriptor resolves no cover, rather than calling undefined", () => { + const workflow = { + id: 10, + type: "workflow", + coverImageUrl: "carried-on-the-entry", + accessibleUserIds: [], + } as unknown as DashboardEntry; + const unregistered = { + id: 12, + type: "computing-unit", + coverImageUrl: "carried-on-the-entry", + accessibleUserIds: [], + } as unknown as DashboardEntry; + component.entities = [workflow, unregistered]; + + expect(() => component.ngOnInit()).not.toThrow(); + expect(coverCache(component).has("workflow:10")).toBe(false); + expect(coverCache(component).has("computing-unit:12")).toBe(false); + expect(component.getCoverImage(workflow)).toBe(component.defaultBackground); + }); + + it("caches nothing when the descriptor resolves an empty cover url", () => { + // A presigned-URL endpoint with nothing to sign answers with an empty string; caching that + // would put an <img src=""> on the card, which the browser resolves to the page itself. + vi.spyOn(TestBed.inject(DatasetService) as any, "getDatasetCoverUrl").mockReturnValue(of({ url: "" })); + const entity = { + id: 11, + type: "dataset", + coverImageUrl: "has-cover", + accessibleUserIds: [], + } as unknown as DashboardEntry; + component.entities = [entity]; + component.ngOnInit(); + + // White-box on purpose: getCoverImage's `|| defaultBackground` makes "cached an empty string" + // and "cached nothing" indistinguishable through the public API, so only the map itself can + // say whether the guard ran. + expect(coverCache(component).has("dataset:11")).toBe(false); + expect(component.getCoverImage(entity)).toBe(component.defaultBackground); + }); + + /** The component's cover cache, which no public member exposes. */ + function coverCache(c: BrowseSectionComponent): Map<string, string> { + return (c as unknown as { coverImageUrls: Map<string, string> }).coverImageUrls; + } }); }); /**
