fix(tests): un-quarantine and fix the 15 unmasked unit failures (#1103)#1338
Merged
Conversation
Removes the @pytest.mark.skip quarantines added in #300 and fixes the underlying issues. Each group fixed at root cause, not by matching assertions to current behavior. Environmental (git identity): - tests/unit/conftest.py: set GIT_AUTHOR/COMMITTER_NAME/EMAIL process-wide so in-test `git commit` works on a CI runner with no global git identity ("Author identity unknown"). Fixes test_reset_preserve_state_guardrails (3) and the test_git_pull_branch end-to-end setups. Test-setup bug (production code was correct): - test_git_pull_branch.py: the repos did `git push -u origin main` but `git init` defaults to `master` (no init.defaultBranch), so origin/main never existed and _get_pull_branch correctly fell back to the working branch — the assertions expecting "main" failed. Force `git init -b main` (local + bare). Fixes all 5 (TestGetPullBranch 2 + TestGitPullFromMainEndToEnd 3). Test-isolation bug (assertions were correct): - test_orphaned_execution_recovery.py: shared module-level mocks were reset with plain reset_mock(), which keeps return_value/side_effect — so one test's get_agent_container.side_effect bled into later tests under random ordering, skewing recovery counts ("assert 3 == 2"). Reset with reset_mock(return_value=True, side_effect=True). Stable across 5 seeds. Real lint findings: - docker/base-image/startup.sh: shellcheck now exits 0. Converted the 4 fragile file-iteration loops to `find -print0 | while read` (SC2010/SC2045/SC2044), hardened 8 `cd` with `|| exit 1` (SC2164), split the SC2155 export, and documented-disabled SC2001 on the two regex `sed` lines that ${//} can't express. `bash -n` clean. Un-skips test_startup_sh_shellcheck_clean. backlog (3) and 929 (1) were already un-quarantined on dev (db_harness schema), so no change needed there. Verified: the 11 un-skipped tests pass across multiple random seeds; full unit suite shows no regressions from these changes (the unrelated pre-existing test_1115_schedules_summary / test_admin_email_login failures fail on dev too). Related to #1103 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vybe
approved these changes
Jun 24, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr: base=dev, conventional commit, root-cause test fixes + shellcheck hardening in startup.sh, security-clean (trinity-agent@ability.ai is a pre-existing bot git-identity, not new PII). All required checks green. Approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes the
@pytest.mark.skipquarantines (added in #300 to keep CI green once the suite ran) and fixes each underlying failure at root cause — not by editing assertions to match current behavior.theme-reliability. 11 markers across 5 files;test_backlog(3)+test_929(1) were already un-quarantined on dev.Fixes
tests/unit/conftest.pysetsGIT_AUTHOR/COMMITTER_*so in-testgit commitworks on a runner with no global git identity._get_pull_branchdrift (5) — production was correct: repospush -u origin mainbutgit initdefaults tomaster, soorigin/mainnever existed → correct working-branch fallback. Fixed withgit init -b main(local + bare).reset_mock()keptside_effect, bleeding across tests under random order ("assert 3==2"). Fixed withreset_mock(return_value=True, side_effect=True); stable across 5 seeds.startup.shnow exit 0 — fragile loops →find -print0|while read(SC2010/2045/2044), 8cd|| exit 1(SC2164), SC2155 export split, SC2001 documented-disable on regexsed.bash -nclean.Verification
79 passed over the 5 affected files × 3 random seeds; shellcheck 0.10.0 exit 0; full unit suite 2755 passed, no regressions (the
test_1115/test_admin_email_loginfailures are pre-existing on dev — reproduce with this branch's conftest stashed).Out of scope (issue follow-ups)
Pre-existing
test_1115/test_admin_email_loginfailures + collection errors (separate triage); Postgres CI service + collection-error CI guard (follow-ups 2 & 3, separate PR).Related to #1103
🤖 Generated with Claude Code