Copilot commented on code in PR #8290:
URL: https://github.com/apache/texera/pull/8290#discussion_r3895122392
##########
frontend/src/app/dashboard/component/user/user-model/user-model-explorer/model-detail.component.spec.ts:
##########
@@ -797,4 +810,439 @@ describe("ModelDetailComponent", () => {
expect(component.isMaximized).toBe(true);
expect(fixture.nativeElement.querySelector(".model-header")).toBeNull();
});
+
+ // ─── the signed-in user
─────────────────────────────────────────────────────
+
+ it("tracks the signed-in user as the session changes under it", () => {
+ // The page is reachable while signed out (a public model), and every
service call
+ // this component makes picks its endpoint off isLogin.
+ create();
+ const users = TestBed.inject(UserService) as unknown as StubUserService;
+ expect(component.isLogin).toBe(true);
+
+ users.user = undefined;
+ users.userChangeSubject.next(undefined);
+
+ expect(component.isLogin).toBe(false);
+ expect(component.currentUid).toBeUndefined();
+
+ const signedIn = { uid: 42, name: "n", email: "e", role: Role.REGULAR } as
User;
+ users.user = signedIn;
+ users.userChangeSubject.next(signedIn);
+
+ expect(component.isLogin).toBe(true);
+ expect(component.currentUid).toBe(42);
+ });
+
+ // ─── the resizable sider
────────────────────────────────────────────────────
+
+ it("stores the dragged sider width on the next animation frame", async () =>
{
+ create();
+ expect(component.siderWidth).toBe(400);
+
+ // A drag emits continuously; the component coalesces to one write per
frame.
+ component.onSideResize({ width: 250, height: 999 });
+
+ // The write is deferred, not immediate — that deferral is the whole point
of the
+ // requestAnimationFrame hop, and a synchronous assignment would land here.
+ expect(component.siderWidth).toBe(400);
+ await nextFrame();
+
+ expect(component.siderWidth).toBe(250);
+ });
+
+ // ─── guards against an absent model id
──────────────────────────────────────
+
+ it("fetches nothing without a model id", () => {
+ create();
+ modelService["getModel"].mockClear();
+ modelService["retrieveModelVersionList"].mockClear();
+ component.mid = undefined;
+
+ component.retrieveModelInfo();
+ component.retrieveModelVersionList();
+
+ // Without the guards these would request /api/model/undefined.
+ expect(modelService["getModel"]).not.toHaveBeenCalled();
+ expect(modelService["retrieveModelVersionList"]).not.toHaveBeenCalled();
+ });
+
+ it("saves nothing and changes nothing on screen without a model id", () => {
+ modelService["updateModelName"] = vi.fn(() => of({}));
+ modelService["updateModelDescription"] = vi.fn(() => of({}));
+ modelService["updateModelFramework"] = vi.fn(() => of({}));
+ modelService["updateModelFormat"] = vi.fn(() => of({}));
+ create();
+ component.mid = undefined;
+ component.editedModelName = "renamed";
+
+ component.onSaveModelName();
+ component.onModelDescriptionChange("changed");
+ component.onFrameworkChange("tensorflow");
+ component.onFormatChange("onnx");
+
+ expect(modelService["updateModelName"]).not.toHaveBeenCalled();
+ expect(modelService["updateModelDescription"]).not.toHaveBeenCalled();
+ expect(modelService["updateModelFramework"]).not.toHaveBeenCalled();
+ expect(modelService["updateModelFormat"]).not.toHaveBeenCalled();
+ // The description, framework and format writes are all optimistic, so a
missing guard
+ // would also leave the page claiming a value that was never persisted.
+ expect(component.modelName).toBe("resnet-50");
+ expect(component.modelDescription).toBe("a description");
+ expect(component.modelFramework).toBe("pytorch");
+ expect(component.modelFormat).toBe("torchscript");
+ });
+
+ // ─── timestamps that are not timestamps
─────────────────────────────────────
+
+ it("leaves the creation time blank when the model carries no usable
timestamp", () => {
+ modelService["getModel"] = vi.fn(() => of(dashboardModel({ model: {
creationTime: undefined } })));
+ create();
+
+ expect(component.modelCreationTime).toBe("");
+ expect(component.modelCreationTimeTooltip).toBe("");
+
+ // A timestamp that arrives as a string is not a timestamp either:
formatting it would
+ // print a date and a time zone the backend never sent.
+ modelService["getModel"] = vi.fn(() => of(dashboardModel({ model: {
creationTime: "2023-11-03T00:00:00Z" } })));
+ create();
+
+ expect(component.modelCreationTime).toBe("");
+ expect(component.modelCreationTimeTooltip).toBe("");
+ });
+
+ it("leaves a version's creation time blank, and its row off the sider,
without a timestamp", () => {
+ modelService["retrieveModelVersionList"] = vi.fn(() =>
+ of([{ ...aVersion(1, "v1"), creationTime: undefined as unknown as number
}])
+ );
+ create();
+
+ expect(component.latestVersionCreationTime).toBe("");
+ expect(component.selectedVersionCreationTime).toBe("");
+ // An em dash here would render "Created at: —"; the absent-timestamp row
is meant to
+ // disappear instead, and the Model Card supplies its own dash.
+ expect(openTab("Versions &
Files").querySelector(".version-date")).toBeNull();
+
+ // A timestamp that arrives as a string is not a timestamp either, and
this guard tests the
+ // type rather than mere presence: a not-undefined check would let the
string through and
+ // print a date the backend never sent.
+ modelService["retrieveModelVersionList"] = vi.fn(() =>
+ of([{ ...aVersion(1, "v1"), creationTime: "2023-11-03T00:00:00Z" as
unknown as number }])
+ );
+ create();
+
+ expect(component.latestVersionCreationTime).toBe("");
+ expect(component.selectedVersionCreationTime).toBe("");
+ expect(openTab("Versions &
Files").querySelector(".version-date")).toBeNull();
+ });
+
+ // ─── failures reaching the user
─────────────────────────────────────────────
+
+ it("reports a file-tree failure for the version being opened", () => {
+ modelService["retrieveModelVersionList"] = vi.fn(() => of([aVersion(1,
"v1")]));
+ modelService["retrieveModelVersionFileTree"] = vi.fn(() => throwError(()
=> new Error("version tree gone")));
+ create();
+
+ expect(notificationService["error"]).toHaveBeenCalledWith("version tree
gone");
+ });
+
+ it("reports a rejected rename and keeps the old name on screen", () => {
+ modelService["updateModelName"] = vi.fn(() => throwError(() => new
Error("name already taken")));
+ create();
+ component.editedModelName = "resnet-101";
+
+ component.onSaveModelName();
+
+ // The rename is not optimistic: the header must not claim a name the
server refused.
+ expect(component.modelName).toBe("resnet-50");
+ expect(notificationService["error"]).toHaveBeenCalledWith("name already
taken");
+ });
+
+ it("reports a failure refreshing the Model Card after a rename", () => {
+ const versions = [aVersion(2, "v2"), aVersion(1, "v1")];
+ modelService["updateModelName"] = vi.fn(() => of({}));
+ modelService["retrieveModelVersionList"] = vi.fn(() => of(versions));
+ create();
+ component.onVersionSelected(versions[1]);
+
+ // The browsed version still resolves; only the newest one — fetched for
the card — does not.
+ modelService["retrieveModelVersionFileTree"] = vi.fn((_mid: number, mvid:
number) =>
+ mvid === 2 ? throwError(() => new Error("latest tree gone")) : of({
fileNodes: [], size: 0 })
+ );
+ notificationService["error"].mockClear();
+ component.editedModelName = "resnet-101";
+
+ component.onSaveModelName();
+
+ expect(notificationService["error"]).toHaveBeenCalledWith("latest tree
gone");
+ });
+
+ it("clears the Model Card facts when no version is left to describe", () => {
+ const versions = [aVersion(2, "v2"), aVersion(1, "v1")];
+ modelService["updateModelName"] = vi.fn(() => of({}));
+ modelService["retrieveModelVersionList"] = vi.fn(() => of(versions));
+ modelService["retrieveModelVersionFileTree"] = vi.fn(() =>
+ of({ fileNodes: [aFile("model.pt", `/model/${OWNER}/resnet-50/v2`)],
size: 512 })
+ );
+ create();
+ component.onVersionSelected(versions[1]);
+ expect(component.latestVersionSize).toBe(512);
+
+ // Every version was deleted from another tab, so the refresh finds
nothing to describe.
+ component.versions = [];
+ component.editedModelName = "resnet-101";
+ component.onSaveModelName();
+
+ // Stale facts here would have the card describing a version that no
longer exists.
+ expect(component.latestVersionCreationTime).toBe("");
+ expect(component.latestVersionFileName).toBe("");
+ expect(component.latestVersionSize).toBeUndefined();
+ });
+
+ it("falls back to the version's first file when the path being reopened is
gone", () => {
+ modelService["retrieveModelVersionList"] = vi.fn(() => of([aVersion(1,
"v1")]));
+ modelService["retrieveModelVersionFileTree"] = vi.fn(() =>
+ of({
+ fileNodes: [
+ aFile("first.txt", `/model/${OWNER}/resnet-50/v1`),
+ {
+ name: "weights",
+ type: "directory" as const,
+ parentDir: `/model/${OWNER}/resnet-50/v1`,
+ children: [aFile("model.pt",
`/model/${OWNER}/resnet-50/v1/weights`)],
+ },
+ ],
+ size: 8,
+ })
+ );
+ create();
+
+ component.onVersionSelected(component.versions[0], "weights/deleted.pt");
+
+ // A miss has to read as a miss: returning whatever the walk last looked
at would open
+ // the nested file instead of the version's first.
+
expect(component.currentDisplayedFileName).toBe(`/model/${OWNER}/resnet-50/v1/first.txt`);
+ });
+
+ it("treats a cleared description as an empty one", () => {
+ modelService["updateModelDescription"] = vi.fn(() => of({}));
+ create();
+
+ component.onModelDescriptionChange(undefined as unknown as string);
+
+ // An undefined body would be written into the column verbatim, and the
+ // did-anything-change comparison below would then never settle.
+ expect(modelService["updateModelDescription"]).toHaveBeenCalledWith(MID,
"");
+ expect(component.modelDescription).toBe("");
+ });
+
+ it("rolls back a rejected framework, and confirms an accepted format", () =>
{
+ modelService["updateModelFramework"] = vi.fn(() => throwError(() => new
Error("unknown framework")));
+ modelService["updateModelFormat"] = vi.fn(() => of({}));
+ create();
+
+ component.onFrameworkChange("tensorflow");
+
+ // The select is written optimistically, so a rejected change has to be
put back or the
+ // page shows a framework the model does not have.
+ expect(component.modelFramework).toBe("pytorch");
+ expect(notificationService["error"]).toHaveBeenCalledWith("unknown
framework");
+
+ // An accepted framework is confirmed under its own label. The two
messages sit twenty
+ // lines apart and differ only in the noun, so each needs pinning at its
own site — a
+ // simultaneous swap of both would otherwise pass for a single one-sided
regression.
+ modelService["updateModelFramework"] = vi.fn(() => of({}));
+ component.onFrameworkChange("tensorflow");
+
+ expect(component.modelFramework).toBe("tensorflow");
+ expect(notificationService["success"]).toHaveBeenCalledWith("Framework set
to 'tensorflow'");
+
+ component.onFormatChange("onnx");
+
+ expect(component.modelFormat).toBe("onnx");
+ expect(notificationService["success"]).toHaveBeenCalledWith("Format set to
'onnx'");
+ });
+
+ it("skips a framework or format change that is already the current value",
() => {
+ modelService["updateModelFramework"] = vi.fn(() => of({}));
+ modelService["updateModelFormat"] = vi.fn(() => of({}));
+ create();
+
+ component.onFrameworkChange("pytorch");
+ component.onFormatChange("torchscript");
+
+ expect(modelService["updateModelFramework"]).not.toHaveBeenCalled();
+ expect(modelService["updateModelFormat"]).not.toHaveBeenCalled();
+ });
+
+ // ─── the template's own listeners, driven from the DOM
──────────────────────
+ //
+ // Every test above calls a handler on the instance, which cannot tell
whether the
+ // template is wired to it at all. These drive the real controls instead.
+
+ const oneVersionWithOneFile = (): void => {
+ modelService["retrieveModelVersionList"] = vi.fn(() => of([aVersion(1,
"v1")]));
+ modelService["retrieveModelVersionFileTree"] = vi.fn(() =>
+ of({ fileNodes: [aFile("model.pt", `/model/${OWNER}/resnet-50/v1`)],
size: 8 })
+ );
+ };
+
+ /** The toolbar buttons carry no classes, only their tooltip text. */
+ const byTooltip = (title: string): HTMLButtonElement => {
+ const found = Array.from((fixture.nativeElement as
HTMLElement).querySelectorAll<HTMLButtonElement>("button")).find(
+ button => button.getAttribute("nz-tooltip") === title
+ );
+ expect(found, `expected a button tooltipped "${title}"`).toBeDefined();
+ return found!;
+ };
+
+ it("wires each preview toolbar button to its own handler", () => {
+ oneVersionWithOneFile();
+ create();
+ const root = openTab("Versions & Files");
+ const writeText = vi.fn(() => Promise.resolve());
+ Object.defineProperty(navigator, "clipboard", { value: { writeText },
configurable: true });
+
+ q<HTMLButtonElement>(root, ".copy-path-btn").click();
+
expect(writeText).toHaveBeenCalledWith(`/model/${OWNER}/resnet-50/v1/model.pt`);
Review Comment:
This test overrides the global `navigator.clipboard` but never restores it,
which can leak into later specs and cause hard-to-diagnose failures (other
files in this repo restore the original descriptor after clipboard stubbing).
Capture the original property descriptor before stubbing and restore (or
delete) it once the copy-path assertion is complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
