Skip to content

[jsweep] Clean update_handler_factory.cjs #42900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
github-actions wants to merge 1 commit into from
Closed

[jsweep] Clean update_handler_factory.cjs #42900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions actions/setup/js/update_handler_factory.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,12 @@ function createStandardFormatResult(fieldMapping) {
const { numberField, urlField, urlSource } = fieldMapping;

return function formatSuccessResult(itemNumber, updatedItem) {
const result = {
return {
success: true,
[numberField]: itemNumber,
[urlField]: updatedItem[urlSource],
Comment on lines 95 to 99
title: updatedItem.title,
};
return result;
};
}

Expand Down
74 changes: 74 additions & 0 deletions actions/setup/js/update_handler_factory.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,80 @@ describe("update_handler_factory.cjs", () => {
expect(captureAfter).toHaveBeenCalledWith({ html_url: "https://example.com/issues/42", title: "Updated title" }, { title: "Before title" }, expect.objectContaining({ title: "Test" }));
});

it("should work when only captureBefore is configured (no captureAfter)", async () => {
const mockResolveItemNumber = vi.fn().mockReturnValue({ success: true, number: 42 });
const mockBuildUpdateData = vi.fn().mockReturnValue({ success: true, data: { title: "Test" } });
const mockExecuteUpdate = vi.fn().mockResolvedValue({ html_url: "https://example.com/issues/42", title: "Updated title" });
const mockFormatSuccessResult = vi.fn().mockReturnValue({ success: true, number: 42 });
const captureBefore = vi.fn().mockResolvedValue({ title: "Before title" });

const handlerFactory = factoryModule.createUpdateHandlerFactory({
itemType: "update_test",
itemTypeName: "test item",
supportsPR: false,
resolveItemNumber: mockResolveItemNumber,
buildUpdateData: mockBuildUpdateData,
executeUpdate: mockExecuteUpdate,
formatSuccessResult: mockFormatSuccessResult,
captureExecutionMetadata: { captureBefore },
});

const handler = await handlerFactory({});
const result = await handler({ title: "Test" });

expect(result.success).toBe(true);
expect(result.before_state).toEqual({ title: "Before title" });
// after_state should not be present when captureAfter is missing
expect(result.after_state).toBeUndefined();
expect(captureBefore).toHaveBeenCalled();
});

it("should work when only captureAfter is configured (no captureBefore)", async () => {
const mockResolveItemNumber = vi.fn().mockReturnValue({ success: true, number: 42 });
const mockBuildUpdateData = vi.fn().mockReturnValue({ success: true, data: { title: "Test" } });
const mockExecuteUpdate = vi.fn().mockResolvedValue({ html_url: "https://example.com/issues/42", title: "Updated title" });
const mockFormatSuccessResult = vi.fn().mockReturnValue({ success: true, number: 42 });
const captureAfter = vi.fn().mockResolvedValue({ title: "After title" });

const handlerFactory = factoryModule.createUpdateHandlerFactory({
itemType: "update_test",
itemTypeName: "test item",
supportsPR: false,
resolveItemNumber: mockResolveItemNumber,
buildUpdateData: mockBuildUpdateData,
executeUpdate: mockExecuteUpdate,
formatSuccessResult: mockFormatSuccessResult,
captureExecutionMetadata: { captureAfter },
});

const handler = await handlerFactory({});
const result = await handler({ title: "Test" });

expect(result.success).toBe(true);
expect(result.after_state).toEqual({ title: "After title" });

@github-actions github-actions Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing symmetric absence assertion: before_state is never asserted absent when only captureAfter is configured, creating an asymmetry with the captureBefore-only test which explicitly asserts expect(result.after_state).toBeUndefined().

💡 Suggested fix

Add after expect(result.after_state).toEqual({ title: "After title" }):

expect(result.before_state).toBeUndefined();

Without this, a regression where attachExecutionState's falsy guard is tightened (e.g. changed from beforeState ? to beforeState !== null ?) and a non-null beforeState leaks into the result would go undetected. The captureBefore-only test correctly covers the symmetric case.

// captureAfter was called with null beforeState since captureBefore was not configured
expect(captureAfter).toHaveBeenCalledWith(expect.objectContaining({ title: "Updated title" }), null, expect.anything());

@github-actions github-actions Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing symmetric assertion: this test checks that after_state is set correctly, but does not assert before_state is absent — unlike its sibling test that explicitly checks expect(result.after_state).toBeUndefined().

💡 Suggested addition

Add after line 404:

// before_state should not be present when captureBefore is not configured
expect(result.before_state).toBeUndefined();

Without this, a regression where attachExecutionState leaks a null / empty before_state field would go undetected.

@copilot please address this.

});

it("should log cross-repo update when item has explicit repo field", async () => {
const mockExecuteUpdate = vi.fn().mockResolvedValue({ html_url: "https://example.com", title: "Updated" });

const handlerFactory = factoryModule.createUpdateHandlerFactory({
itemType: "update_test",
itemTypeName: "test item",
supportsPR: false,
resolveItemNumber: vi.fn().mockReturnValue({ success: true, number: 42 }),
buildUpdateData: vi.fn().mockReturnValue({ success: true, data: { title: "Test" } }),
executeUpdate: mockExecuteUpdate,
formatSuccessResult: vi.fn().mockReturnValue({ success: true }),
});

const handler = await handlerFactory({ "target-repo": "other-owner/other-repo", allowed_repos: ["other-owner/other-repo"] });
await handler({ title: "Test", repo: "other-owner/other-repo" });

expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Cross-repo update"));

@github-actions github-actions Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weak cross-repo assertions: Two gaps make this test unable to catch silent cross-repo routing failures.

💡 Suggested fixes

1. Pin the target repo in the log assertion:

The current assertion accepts any string containing "Cross-repo update":

expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Cross-repo update"));

If resolveAndValidateRepo resolved to the wrong repo (e.g. default context repo), the log message would still contain "Cross-repo update" and the test would pass. Change to:

expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Cross-repo update: targeting other-owner/other-repo"));

2. Assert result.repo reflects the cross-repo target:

The handler always attaches repo: ${effectiveContext.repo.owner}/${effectiveContext.repo.repo}`` to the success result. If effectiveContext.repo is not overridden (cross-repo routing silently broken), `result.repo` would contain the workflow's own repo. Capture and assert it:

const result = await handler({ title: "Test", repo: "other-owner/other-repo" });
expect(result.repo).toBe("other-owner/other-repo");

@github-actions github-actions Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The cross-repo log test asserts only that core.info was called with a string containing "Cross-repo update", but the production code also conditionally logs when repoResult.repo !== workflowRepo (i.e. with no repo field). A complementary test for the implicit path would make the logging contract explicit.

💡 Suggested additional test 
it('should log cross-repo update when resolved repo differs from workflow repo (no explicit repo field)', async () => {
  const handlerFactory = factoryModule.createUpdateHandlerFactory({ ... });
  // pass target-repo only — no item.repo field
  const handler = await handlerFactory({ 'target-repo': 'other-owner/other-repo', allowed_repos: ['other-owner/other-repo'] });
  await handler({ title: 'Test' }); // no repo field

  expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining('Cross-repo update'));
});

This covers the second branch of the condition at line 189 of update_handler_factory.cjs.

@copilot please address this.

});

it("should pass additional config to log message", async () => {
const mockResolveItemNumber = vi.fn().mockReturnValue({ success: true, number: 42 });
const mockBuildUpdateData = vi.fn().mockReturnValue({ success: true, data: { title: "Test" } });
Expand Down
Loading