This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 30681cd384 test(frontend): close the user, hub and resource-registry
service gaps (#8336)
30681cd384 is described below
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;
+ }
});
});
/**
