[FIX] Record the SHA CI actually built for PR runs - #1191
[FIX] Record the SHA CI actually built for PR runs#1191NaitikVerma6776 wants to merge 5 commits into
Conversation
Keep Test.commit as the PR head and store GitHub's merge ref (and the binary Git commit line from logs) in built_commit so the UI no longer claims we tested a commit that never ran. Co-authored-by: Cursor <cursoragent@cursor.com>
| """Update Test.built_commit from a ``Git commit:`` line in uploaded logs.""" | ||
| try: | ||
| with open(log_path, encoding='utf-8', errors='replace') as handle: | ||
| text = handle.read() |
There was a problem hiding this comment.
This is not going to work very well if it's a very large log file. Can you adjust so that it either reads in batches and returns as soon as it found something?
Stop reading as soon as the Git commit line is found so large VM logs are not loaded in full. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@canihavesomecoffee ....Good catch , it now scans in 64KB batches and stops at the first Git commit: line, with overlap so a split line is still found. Please review the changes. |
Mypy treats webhook merge_commit_sha as Any | None, so strip() failed the type check even after hash validation.
1bf5de0 to
9a012de
Compare
Capture expected SHAs before the GET so assertions do not touch a Test instance after Flask tears down the session.
The logupload unit test mocks os.path.join, so the new Git-commit parser received a MagicMock path. open() treated that as a file descriptor and closed stdout, which made CI exit 120 after the suite had already passed.
|
|
hi @canihavesomecoffee CI is now passing on the latest commit. The latest commit only addresses the CI/test issue around the mocked log path; the approved #1176 implementation remains unchanged. Could you please re-review and approve the current HEAD when you get a chance? Thanks! |
2 participants
Fixes #1176
What was wrong
PR runs store the PR head SHA in
Test.commit. GitHub Actions builds the merge ref (PR head merged into master), so the binary that actually ran is a different commit.Run 9490 is the example Willem posted:
sp run show 9490reports head6e1e22a3b8sp run logs 9490 --allhasGit commit: e98f1a2f81The UI/API therefore claimed we tested a commit that never went through the VM.
What this PR does
commitas the PR head (the GitHub status / PR link still make sense).built_commit.merge_commit_shawhen the PR webhook creates the run.Git commit:line when the VM uploads logs (ground truth from the binary).built_commit_shaon GET /api/v1/runs.PR head …, tested as …on the run page when the two SHAs differ.Master/commit runs are unchanged (
built_commitstays null unless logs say otherwise).Test plan
sp run show <pr-run>commit_shavsbuilt_commit_shaagainstsp run logs <id> --all | grep 'Git commit'