Make /status list every missing item, including pending checks - #288
upsun-dispatch[bot] wants to merge 1 commit into
Conversation
/status already existed (BERTE-539), but it named only the worst failing build, hid bypass notes and left out rows it couldn't check yet. It now lists every failing build, marks unchecked rows as pending, and reports the wait option and merge-queue state. Dispatch-Run: 01M3S829EH1E08T79VVGG1Z0EN Dispatch-Attempt: 1
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · ⚪ 1 nitpick
🔍 Full review · 5 files reviewed
⚪ Nitpick
bert_e/workflow/gitwaterflow/commands.py:66—bypassedlists every active bypass option, whether or not it was needed. Ifneed_author_approvalis false, orrequired_peer_approvals/required_leader_approvalsis 0, but the matchingbypass_*_approvalis set, the row still says e.g. "author/peer approval bypassed". The bypass changed nothing in that case, so the note wrongly suggests the approvals are satisfied only because of it.
Verification
get_build_url(revision, key)exists with this signature on the base, mock, GitHub and Bitbucket hosts, so the new call in_check_builds_statuscannot raise TypeError or AttributeError._check_queue_statusruns only afterbuild_branch_cascade, soalready_in_queuecan readjob.git.cascade.dst_branchessafely.- The template checks
pendingbeforepass, so a pending item built withpass=Falserenders as ⌛ and not as ❗. - Help text comes from
normalize_whitespace(handler.__doc__), so the two-linestatusdocstring shows as the single sentence the new test expects.
The diff adds six integration tests in bert_e/tests/test_bert_e.py (mock git host) and unit tests in bert_e/tests/unit/test_status_report.py for each row and how each state renders. The build-link format is covered only by the mocked unit test, and no test covers a partial approval bypass.
Review details
- Commit: deaee51
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
| leader_count >= required_leader and | ||
| not job.settings.unanimity): | ||
| return _StatusItem('Approvals', True) | ||
| bypassed = [ |
There was a problem hiding this comment.
🟡 Warning — Partial bypasses stay invisible in the report, contrary to the stated feature.
The "approval bypassed" note is only built in the early-return branch, and that branch runs only when the bypasses alone meet every requirement. Take a partial bypass: bypass_peer_approval is set, need_author_approval is true, and the author approved in the UI rather than with approve. author_ok starts out False, so the code skips the early return and falls through to the counting path. That path ends with _StatusItem('Approvals', not details, details=details) with empty details, so the row renders as a plain ☀️. Nothing tells the reader that the peer requirement was met only because of the bypass. The PR says the row now reports when a bypass is what makes approvals pass, but it only does so when all the needed approvals are bypassed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
==========================================
+ Coverage 90.21% 90.33% +0.11%
==========================================
Files 82 83 +1
Lines 11293 11433 +140
==========================================
+ Hits 10188 10328 +140
Misses 1105 1105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
The plan assumed
/statuswas only a "TBA" placeholder, but it isn't. The first step (checking what already exists) found that commitb35e425/ #\276 (BERTE-539) already added a working per-pull-request report. It covers approvals, integration builds, Jira fix versions and history, and has 20 tests. So, as the plan said to do in that case, this PR only fills the gaps between that report and the plan's scenarios.Gaps closed, all in
bert_e/workflow/gitwaterflow/commands.py:[build](url)link when the git host provides a URL./statusstill creates nothing.bypass_*_approvaloption is what makes approvals pass, the row now says so ("author/peer/leader approval bypassed").waitrow: reports thewaitoption as a blocker when it is set.queuerow: shown when queues are enabled and integration branches exist, as "queued" or ⏳ "not queued yet".statusnow reads "Print everything still missing before this pull request can be merged."Other changes:
bert_e/templates/status_report.md: adds the ⏳ "not yet evaluated" state and shows notes on passing rows, so "bypassed" becomes visible (before, a passing row showed only ☀️). A short legend appears only when the report has rows, so the "not available" fallback ininit.mdis unchanged.bert_e/docs/USER_DOC.md: addsstatusto the command table and a short section describing the report. It also says it is separate from the web status page.bert_e/tests/test_bert_e.pyand a new unit test file,bert_e/tests/unit/test_status_report.py, covering each row's logic and how each state renders.Decisions a reviewer should know about:
run_checksrefactor ingitwaterflow/__init__.py. The existing report already runs its checks one by one without stopping at the first failure, and the plan said to add only what was missing. Leaving the fail-fast merge path alone also avoids the regression risk the plan itself flagged.exceptions.py:StatusReportalready exists and is never de-duplicated.templates/status.md: already exists.templates/help.md: generated from the command registry, so the new help text flows through on its own.README.md: doesn't list commands; the command docs live inUSER_DOC.md.bypass_build_statusis set, the builds row now shows as bypassed even before integration branches exist.Testing:
TestBertE.test_status_command*tests (old and new) plustest_options_and_commandspass on the mock git host, both with and without--disable-queues.bert_e/tests/unitrun has 1 failure and 3 errors, all in the GitHub API tests (test_github_app_auth.py,test_github_build_status.py). They need a mock server onlocalhost:4010, which wasn't running. I only confirmed on a clean checkout that thetest_github_app_auth.pyerrors happen without my changes; I didn't recheck thetest_github_build_status.pyfailure the same way.Files changed
bert_e/docs/USER_DOC.md(modified, +24/-0)bert_e/templates/status_report.md(modified, +3/-1)bert_e/tests/test_bert_e.py(modified, +96/-0)bert_e/tests/unit/test_status_report.py(added, +88/-0)bert_e/workflow/gitwaterflow/commands.py(modified, +84/-30)Pre-merge review
bert_e/tests/unitrun had 1 failure and 3 errors. Only thetest_github_app_auth.pyerrors were confirmed to also happen on a clean checkout. Thetest_github_build_status.pyfailure was not rechecked. That file covers the same build-status area as the newjob.project_repo.get_build_url(commit, key)call in_check_builds_status. A reviewer should run it with the mock server onlocalhost:4010before merging, to rule out a regression._check_builds_status. The builds row now callsget_build_urlonce for every integration branch whose build isn't successful, on top ofget_build_status. On long cascades where many builds fail, that is one extra host API call per branch. The plan listed rate limits as a risk. Also, the mock host returns no build URLs, so only the unit test (which mocksproject_repo) exercises the link format. Nothing checks that every real git host backend hasget_build_url(sha, key)with this signature. If one doesn't,/statuswill raiseAttributeErrororTypeErroron that host whenever a build is not green.clone_git_repoorbuild_branch_cascadefails,_build_status_reportstill quietly falls back to a report with approvals only. The plan's scenario says unexpected non-template errors should surface rather than be hidden in the report. Second, a missing Jira id is still shown under the fix-versions row instead of its own row. Both are existing behaviours, but a maintainer should decide whether they are acceptable for this issue.Addresses #280 — implemented according to the plan posted on the issue thread.
Created by Upsun Dispatch™. Requested by @charlesprost.