Copilot commented on code in PR #8133:
URL: https://github.com/apache/texera/pull/8133#discussion_r3894098925
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -66,6 +67,10 @@ describe("ShareAccessComponent", () => {
let workflowActionSpy: { setWorkflowIsPublished: ReturnType<typeof vi.fn> };
let userServiceCurrentEmail: string | undefined;
let capturedModalConfigs: any[];
+ /** The NzModalRef stubs handed back by modalService.create, in creation
order. */
+ let capturedModalRefs: { close: ReturnType<typeof vi.fn> }[];
+ /** The fixture built by the most recent setupComponent() call, for the
template-level tests. */
+ let fixture: ComponentFixture<ShareAccessComponent>;
function setupComponent(opts: SetupOptions = {}): ShareAccessComponent {
Review Comment:
Storing `fixture` in shared mutable state increases coupling between tests
and makes future parallelization/concurrency harder. A more maintainable
pattern is for `setupComponent` to return both `{ fixture, component }` (or
return the fixture and read `fixture.componentInstance`), so each test keeps
its own local references without relying on cross-test globals.
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -711,4 +719,266 @@ describe("ShareAccessComponent", () => {
expect(datasetServiceSpy.updateDatasetPublicity).not.toHaveBeenCalled();
});
});
+
+ /**
+ * Everything above drives the component's methods directly. The template
decides which control
+ * reaches which of those methods, and with what argument — the publish pair
and the per-row
+ * access controls are near-symmetric, so a crossed binding would look right
on screen and do the
+ * opposite thing.
+ */
+ describe("template wiring", () => {
+ /** Makes the current user the owner, which is what enables the
write-gated controls. */
+ function asOwner(): void {
+ accessServiceSpy.getOwner.mockReturnValue(of("[email protected]"));
+ }
+
+ it("puts the unpublish confirmation behind Private and the publish
confirmation behind Public", () => {
+ asOwner();
+ workflowPersistSpy.getWorkflowIsPublished.mockReturnValue(of("Private"));
+ setupComponent({ type: "workflow" });
+
+ const [privateButton, publicButton] =
fixture.debugElement.queryAll(By.css("button.access-button"));
Review Comment:
This assertion depends on DOM order (`[privateButton, publicButton]`), which
is brittle and can break with harmless template refactors or styling/layout
changes. Make the selection resilient by locating the buttons by their visible
text/label, aria-label, or adding a dedicated test id (e.g., data-testid) so
the test asserts the intended mapping (Private vs Public) rather than their
position.
##########
frontend/src/app/workspace/component/workspace.component.spec.ts:
##########
@@ -380,6 +380,48 @@ describe("WorkspaceComponent", () => {
vi.useRealTimers();
}
});
+
+ it("does not persist an edit made by a signed-out visitor", async () => {
+ // A guest can still edit the canvas; persisting on their behalf would
write to whatever
+ // workflow id the URL happens to carry.
+ vi.useFakeTimers();
+ try {
+ const workflowChanged$ = new Subject<void>();
+ await createFixture();
Review Comment:
Using fake timers before `await createFixture()` can make Angular’s async
compilation/test setup brittle (fake timers can interfere with async scheduling
during `compileComponents()` and other setup work). Prefer moving
`vi.useFakeTimers()` to after `await createFixture()` (and after any async
setup), so only the debounce/timeout behavior under test is timer-controlled.
##########
frontend/src/app/dashboard/component/user/user-workflow/user-workflow-list-item/user-workflow-list-item.component.spec.ts:
##########
@@ -434,6 +493,37 @@ describe("UserWorkflowListItemComponent rendering", () => {
expect(deleted).toHaveBeenCalledTimes(1);
expect(duplicated).toHaveBeenCalledTimes(1);
});
+
+ it("opens the executions modal from the history action", async () => {
+ await setup({ executionsTracking: true });
+ render(makeWorkflowEntry({ wid: 11, name: "wf" }));
+ const modal = TestBed.inject(NzModalService);
+ const create = vi.spyOn(modal, "create").mockReturnValue({} as any);
+
+ byTooltip(t => t.startsWith("Executions of the workflow"))[0].click();
+
+ expect(create).toHaveBeenCalledWith(
+ expect.objectContaining({ nzContent:
WorkflowExecutionHistoryComponent, nzData: { wid: 11 } })
+ );
Review Comment:
Selecting UI elements by tooltip text is fragile (copy changes, i18n,
punctuation changes). Prefer selecting the history action by a stable hook
(data-testid), an accessible label, or a component/directive selector so the
test validates behavior without being tightly coupled to user-facing wording.
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.spec.ts:
##########
@@ -92,14 +97,15 @@ describe("ShareAccessComponent", () => {
{ provide: WorkflowActionService, useValue: workflowActionSpy },
],
});
- const fixture = TestBed.createComponent(ShareAccessComponent);
+ fixture = TestBed.createComponent(ShareAccessComponent);
Review Comment:
Storing `fixture` in shared mutable state increases coupling between tests
and makes future parallelization/concurrency harder. A more maintainable
pattern is for `setupComponent` to return both `{ fixture, component }` (or
return the fixture and read `fixture.componentInstance`), so each test keeps
its own local references without relying on cross-test globals.
--
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]
