aglinxinyuan commented on code in PR #8133:
URL: https://github.com/apache/texera/pull/8133#discussion_r3896208652
##########
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:
Refused, with the premise checked against this runner's config.
`frontend/vitest.config.ts` sets no `fakeTimers` block, so Vitest's default
`toFake` list applies: `setTimeout`/`clearTimeout`,
`setInterval`/`clearInterval`, `setImmediate`/`clearImmediate` and `Date` —
**not** `queueMicrotask` and not `process.nextTick`. The only `await` inside
`createFixture` is `TestBed.configureTestingModule(...).compileComponents()`,
which resolves on the microtask queue.
So the clock cannot interfere with fixture setup, and installing it first
*does* keep any timer the component schedules during creation on the fake
clock. The current order is the more hermetic one.
##########
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:
Applied in `9154c653` — and it cost something I had to put back in
`3a7d25f4`, which is worth recording here.
The destructure is replaced by an `accessButton("Private" | "Public")`
helper that filters `button.access-button` by its visible `.button-text-header`
text and asserts the match is unique. No `data-testid` was added; that label
already exists in the shipped template.
**But the positional destructure had been carrying a DOM-order constraint
implicitly**, and dropping it let a mutant that swaps the two buttons in the
template survive. Rather than revert the selector, I added a test that reads
the two labels in document order and asserts `["Private", "Public"]`. Order and
identity now fail independently, so the fragility you flagged is gone *and* the
ordering contract is explicit instead of accidental.
##########
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:
Applied in the narrow sense, in `9154c653`: the shared `fixture` is now
cleared in the existing `beforeEach`, alongside the
`TestBed.resetTestingModule()` already there, with a note on the declaration.
The `setupComponent` → `{ fixture, component }` refactor is refused. Its
stated benefit is future parallelisation, and that is not available here: the
Angular unit-test builder forks one Vitest worker per spec **file**, and specs
within a file run sequentially against a single module registry. Returning the
fixture would change nothing about concurrency, leaving a style change that
touches every call site in a 780-line spec.
##########
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:
Same thread as the comment at line 79 — see the reply there. Short version:
the shared `fixture` is now cleared in `beforeEach` (`9154c653`), and the `{
fixture, component }` return is refused because one worker runs one spec file's
tests sequentially, so there is no concurrency for it to unlock.
##########
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:
Applied in part, in `9154c653`. Added `onlyByTooltip(pred)` — it wraps the
pre-existing `byTooltip()` and asserts exactly one match — and used it at the
three sites this PR introduces, so a copy change that makes the predicate
ambiguous now fails loudly instead of silently picking the first hit.
The `data-testid` / accessible-label suggestion is refused: both are
production template edits, and this PR is test-only with an empty production
diff, which is the basis it was approved on. The one non-production alternative
is `nzType="history"`, and it is not more stable — it is a template attribute
exactly like the tooltip, and it pins the glyph rather than the text a user
actually reads.
--
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]
