refactor: extract pushSignedCommits to shared helper, add integration tests#21584
Conversation
…ests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🟢 Looking good, Everything looks aligned with the project's contribution guidelines and the agentic workflow chain (issue #21572 → PR #21576 → this sub-PR). Ready for maintainer review. 🎉
|
There was a problem hiding this comment.
Pull request overview
Refactors duplicated signed-commit push logic into a shared helper and adds an integration test suite to validate GraphQL commit replay and git-push fallback behavior.
Changes:
- Added
push_signed_commits.cjsshared helper exportingpushSignedCommits. - Updated
create_pull_request.cjsandpush_to_pull_request_branch.cjsto use the shared helper (and removed now-unused imports). - Added
push_signed_commits.test.cjsintegration tests using a local bare git repo as the remote and a mocked GraphQL layer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| actions/setup/js/push_to_pull_request_branch.cjs | Removes inline duplicate pushSignedCommits and imports shared helper. |
| actions/setup/js/create_pull_request.cjs | Removes inline duplicate pushSignedCommits and imports shared helper. |
| actions/setup/js/push_signed_commits.cjs | New shared implementation of pushSignedCommits. |
| actions/setup/js/push_signed_commits.test.cjs | New integration tests validating GraphQL payloads and fallback behavior. |
Comments suppressed due to low confidence (1)
actions/setup/js/push_signed_commits.cjs:84
- Same issue for added/modified paths: reading
parts[1]from the working tree means replayed commits won’t faithfully represent intermediate states when a file changes across commits. Please source the blob content from the commit being replayed (sha) rather than from the checkout filesystem.
const content = fs.readFileSync(path.join(cwd, parts[1]));
additions.push({ path: parts[1], contents: content.toString("base64") });
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const content = fs.readFileSync(path.join(cwd, parts[2])); | ||
| additions.push({ path: parts[2], contents: content.toString("base64") }); |
| } else if (status.startsWith("R") || status.startsWith("C")) { | ||
| // Rename or Copy: parts[1] = old path, parts[2] = new path | ||
| deletions.push({ path: parts[1] }); | ||
| const content = fs.readFileSync(path.join(cwd, parts[2])); | ||
| additions.push({ path: parts[2], contents: content.toString("base64") }); |
| } catch (graphqlError) { | ||
| core.warning(`pushSignedCommits: GraphQL signed push failed, falling back to git push: ${graphqlError instanceof Error ? graphqlError.message : String(graphqlError)}`); | ||
| await exec.exec("git", ["push", "origin", branch], { | ||
| cwd, | ||
| env: { ...process.env, ...(gitAuthEnv || {}) }, | ||
| }); | ||
| } |
| const headlines = githubClient.graphql.mock.calls.map(c => c[1].input.message.headline); | ||
| expect(headlines).toEqual(["Add file-a.txt", "Add file-b.txt"]); | ||
| }); | ||
|
|
pushSignedCommitswas duplicated verbatim in bothcreate_pull_request.cjsandpush_to_pull_request_branch.cjs. This PR extracts it to a shared helper and adds an integration test suite.Changes
push_signed_commits.cjs(new) — shared helper exportingpushSignedCommits; both callers nowrequireit from herecreate_pull_request.cjs/push_to_pull_request_branch.cjs— remove the ~92-line inline duplicate each; drop the now-unusedpathimportpush_signed_commits.test.cjs(new) — 8 integration tests using a real bare git repo as a local "remote"; GraphQL layer mockedIntegration tests cover:
fileChanges.deletionsfileChangeshas no additions/deletionsgit push; remote HEAD confirmed vials-remotemessage.bodypopulatedmessage.bodyisundefined✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.