feat: test creation and commit in /tests - #13
Conversation
|
Automated Code Review Comment: Bugs
Improvements
Suggestions
Suggested Testsimport pytest def test_commit_tests(tmpdir): def test_get_pr_diff(): def test_generate_tests(): def test_generate_tests_empty_diff(): def test_main(tmpdir, monkeypatch): |
There was a problem hiding this comment.
Pull request overview
This PR extends the apricot agent so it can not only generate suggested tests, but also write them into /tests/test_generated.py and attempt to commit/push them back to the repository from the GitHub Actions workflow.
Changes:
- Save LLM-generated tests to
tests/test_generated.pyand invoke a new commit/push helper fromagent/main.py. - Add
agent/github/committer.pyto git-add/commit/push the generated test file. - Update the workflow to grant
contents: writeand adjust test generation output stripping for fenced code blocks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
agent/main.py |
Writes generated tests to disk and calls commit_tests() after commenting. |
agent/llm/test_generator.py |
Attempts to strip markdown code fences from LLM output before returning tests. |
agent/github/committer.py |
New helper to configure git, add/commit, and push generated tests. |
.github/workflows/agent.yml |
Elevates workflow token permissions to allow writing repository contents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from agent.llm.test_generator import generate_tests | ||
|
|
||
| import subprocess | ||
| from agent.github.committer import commit_tests |
There was a problem hiding this comment.
commit_tests is imported from agent.github.committer, but the new module added in this PR is agent/github/committer.py (double “t”). As-is, the agent will crash with ModuleNotFoundError at startup. Update the import to match the actual filename/module, or rename the module for consistency.
| from agent.github.committer import commit_tests | |
| from agent.github.commiter import commit_tests |
| tests = generate_tests(diff, context=relevant_chunks) | ||
| os.makedirs("tests", exist_ok=True) | ||
| with open("tests/test_generated.py", "w", encoding="utf-8") as f: | ||
| f.write(tests) # Save generated tests to a file for potential commit | ||
| print("Generated tests saved to tests/test_generated.py") |
There was a problem hiding this comment.
The generated test content is written directly to tests/test_generated.py even when generate_tests() returns an error string (it returns "Error generating tests:..." on exceptions). This can lead to committing a non-test error message into the repository. Consider checking for the error sentinel (or otherwise validating that the output is Python test code) before writing/committing.
| # 11. Commit tests | ||
| print("Committing tests...") | ||
| commit_tests() |
There was a problem hiding this comment.
commit_tests() is called unconditionally in the PR workflow. Combined with the workflow trigger on pull_request and a constant commit message, this can create repeated commits / reruns, or attempts to commit when nothing changed. Add a guard (e.g., only commit when tests/test_generated.py differs, and/or skip when the actor is the actions bot / commit message matches).
| content =response.choices[0].message.content.strip() | ||
| if content.startswith("```"): | ||
| content = content.split("```")[1] # Extract code from markdown | ||
| if content.startswith("python"): | ||
| content = content[len("python"):] # Remove language specifier | ||
| return content.strip() |
There was a problem hiding this comment.
Markdown code-fence stripping is fragile here: content.split("```")[1] will raise/behave incorrectly when the response contains multiple fenced blocks, leading text before the fence, or an unclosed fence. Also, the PR description mentions handling bash, but this code only strips python and will leave a leading bash token that will make the saved file invalid Python. Consider robustly parsing a single fenced block (and rejecting/handling non-Python languages) before returning.
| # add the generated test file | ||
| subprocess.run(["git","add","tests/test_generated.py"],check=True) | ||
| # commit the changes | ||
| subprocess.run(["git","commit","-m","Add AI-generated tests"],check=True) | ||
| # push | ||
| subprocess.run(["git","push"],check=True) | ||
| print("Tests committed and pushed successfully.") |
There was a problem hiding this comment.
This function will likely fail to push in the current workflow: on pull_request events, actions/checkout typically checks out a detached merge ref (e.g. refs/pull/*/merge), so git commit/git push will not update the PR branch. If the intent is to push to the PR’s head branch, the workflow needs to check out github.head_ref (and the push should target that branch explicitly).
| subprocess.run(["git","push"],check=True) | ||
| print("Tests committed and pushed successfully.") | ||
| except Exception as e: | ||
| print(f"Commit Failed {e}") |
There was a problem hiding this comment.
commit_tests() swallows all exceptions and only prints Commit Failed ..., which can make the workflow appear successful even though nothing was committed/pushed. Consider returning a status / re-raising after logging so CI accurately reflects whether the commit step worked.
| print(f"Commit Failed {e}") | |
| print(f"Commit Failed {e}") | |
| raise |
| import subprocess | ||
| def commit_tests(): | ||
| try: | ||
| # configue git (required in github actions) |
There was a problem hiding this comment.
Typo in comment: “configue git” should be “configure git”.
| # configue git (required in github actions) | |
| # configure git (required in github actions) |
| permissions: | ||
| pull-requests: write | ||
| contents : read | ||
| contents : write |
There was a problem hiding this comment.
Changing contents permission to write enables pushing commits from this workflow. On pull_request triggers this has two concrete risks: (1) the push can retrigger the workflow and create a commit loop unless you add explicit safeguards, and (2) for fork PRs the token won’t be able to push anyway, so the job will repeatedly attempt/fail. Consider adding if: guards (e.g., only run on non-fork PRs and skip when github.actor is the actions bot), and ensure checkout/push targets the PR head branch if committing is intended.
|
all good @haddybhaiya |
|
Automated Code Review Comment: Bugs
Improvements
Suggestions
Code does not look good due to identified bugs and potential improvements. Suggested Testsimport pytest def test_commit_tests(): def test_get_pr_diff_empty(): def test_commit_tests_exception(): def test_get_pr_diff_no_diff(): def test_commit_tests_push(): |
feat: test creation and commit in /tests
Apricot current release allows only test creation and commenting on PR, while this pull request allows apricot to generated tests and commit them onto
/testswithtest_generated.pyfile name.- key changes made:
readtowritemain.pycallscommit_tests()-
test_generator.pychecks if tests starts with bash (```) or python keyword