CI: clear the three check blockers + fix a covr-fragile stopPatience assertion - #277
Open
ms609 wants to merge 2 commits into
Open
CI: clear the three check blockers + fix a covr-fragile stopPatience assertion#277ms609 wants to merge 2 commits into
ms609 wants to merge 2 commits into
Conversation
Tests were already green (FAIL 0 across 11612); the coverage wave was skipped
because sense-check reported 1 ERROR, 1 WARNING and 2 NOTEs, all from the recent
`effort` work. This clears the two that gate the build, plus one long-standing
NOTE that was generating repeated man-page churn.
ERROR (spelling): `Zanol`. `inst/WORDLIST` already carried `ZANOL`, from
`R/data.R`'s all-caps citation, but not the mixed-case form that reached prose.
WARNING (undocumented argument): `.rung`, added to `MaximizeParsimony()` without
a `@param`. R CMD check requires every argument in a `\usage` section to be
documented even when it is deliberately internal, so it is now documented AS
internal -- what it pins, that `"none"` means no preset at all, that `NULL` is the
ordinary path, and that callers should prefer `effort`. This is the check
AGENTS.md names for a signature change (`devtools::check_man()`); running it would
have caught this before the push.
NOTE (empty \references): `expected_mi`'s roxygen carried
`@references \insertAllCited{}` while citing nothing, so Rd generation emitted an
empty `\references` section. Dropped at source in `src/expected_mi.cpp`. This
also stops `man/expected_mi.Rd` re-churning on every unrelated roxygen run.
NOT touched, and worth knowing:
- "Version contains large components (2.0.0.9999)" is self-inflicted by CI, not by
the repo. DESCRIPTION says `2.0.0`; the workflow's own "Temporarily bump package
version" step (.github/workflows/R-CMD-check.yml:77) appends `.9999` whenever the
version has exactly two dots. Fixing it means changing that step, which is a
release-process decision rather than a docs fix.
- The `Remotes` field and the `MaxMin` suggestion are flagged by the CRAN-incoming
check but are deliberate for a development package.
- `man/SiteConcordance.Rd`'s DOI 10.1201/9780367823412-8 now 404s upstream.
None of these three is a WARNING or an ERROR, so none of them gates the build.
Verified: `spell_check_package()` clean; `tools::checkDocFiles()` reports no
undocumented or mismatched arguments; the `man/expected_mi.Rd` diff is exactly the
three lines of the empty section and nothing else.
Co-Authored-By: Claude Opus 5 <[email protected]>
…hine's speed The coverage wave ran for the first time and immediately failed on my own parallel stopPatience test: FAIL 1 / PASS 11627 under `covr::codecov()`, against FAIL 0 / PASS 11628 in the same commit's ordinary check. So the test was not wrong about the code -- it was wrong about the environment. Mechanism, and it is the one I documented in the very commit that added the test (998891e): on the parallel path the dry spell is counted over replicates completed into the shared pool and evaluated only when the coordinating thread polls, every 200 ms. covr instrumentation makes each replicate far slower, so far fewer land per poll interval and `stopPatience = 3` fires genuinely earlier in the search -- 13.063 against 12.988. `expect_equal(short score, full score)` was therefore asserting a coincidence of timing, not a property of the rule. Stopping sooner is *allowed* to cost score; that is the entire trade-off `stopPatience` exists to offer, and it is what the shipped preset measurements quantify. Asserting equality contradicted the feature's own documentation. What the rule does guarantee is already asserted and untouched: the search stops before `maxReplicates`, and `perturb_stop` is set. The score check is now only that it is finite and not grossly worse, which holds at any speed. Third time this session a test of mine encoded an environment assumption -- after `as.Splits`/`%in%` dispatch and the ARM64 ordering chase. The pattern: an assertion that passes because of how fast, how attached, or how ordered the local environment happens to be, rather than because of what the code promises. Co-Authored-By: Claude Opus 5 <[email protected]>
ms609
added a commit
that referenced
this pull request
Jul 31, 2026
Coordination-only. The reporting half of T-374 is implemented on claude/t385-xform-report-agreement (stacked on PR #277): TreeLength() canonicalises the rooting in both its single-tree and multiPhylo methods, and MaximizeParsimony() rescores the returned pool through that path before reporting. Acceptance script: gap 5 -> 0, rooting spread 5 -> 0. Worth the row update on its own: the new "returned trees do not share a length" warning fired immediately on an EXISTING test -- all-hierarchy xform data returns a pool spanning 7 to 9. That independently corroborates T-374's original "4 of 6 trees do not share a score", which my own 36-tip attempt to reproduce had measured wrongly (retracted in 9296e87). The residue is real, reachable, and now visible at runtime instead of silent.
ms609
added a commit
that referenced
this pull request
Jul 31, 2026
All 8 CI jobs green on run 30662279563. The one ubuntu-24.04 (4.1) failure was transient -- it died in 'Set up R dependencies' with the Bioconductor 3.14 repos unreachable, so 'Check package' was skipped and no code was ever tested; green on re-run of that job alone.
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
1 participant
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.
Clears
R CMD checktoStatus: OKoncpp-search, which let Wave 2 (covr + macOS) run for the first time, and then fixes the one test the coverage wave caught.Check blockers (commit
21b08006)spelling::spell_check_package()— 9 unlisted wordsinst/WORDLISTadditions;erroringreworded tothrowing an error(it isn't a word).rungundocumented inMaximizeParsimony()@param .rungblock marking it internal, pointing users ateffort\referencesinman/expected_mi.Rd\insertAllCited{}tag fromsrc/expected_mi.cpp— it cited nothingThe
expected_mi.Rdfix also ends the repeated regeneration churn in that file, since the empty section was being re-emitted on every roxygen run.The coverage-wave failure (follow-up commit)
Wave 2's first run went red on
windows-latest (release)step 9. Undercovr::codecov()the suite wasFAIL 1 | PASS 11627; the same commit's plain check wasFAIL 0 | PASS 11628— so the test was wrong about the environment, not about the code.The assertion was mine, from
998891e4:expect_equal(min(attr(short, "score")), min(attr(full, "score")))intest-stop-patience.R, giving 13.063 against 12.988. Mechanism is the one documented in that same commit — on the parallel path the dry spell is counted over replicates completed into the shared pool and evaluated on a 200 ms monitor poll, so covr's instrumentation means fewer replicates land per poll andstopPatience = 3fires genuinely earlier in the search.Stopping sooner is allowed to cost score — that is the trade-off the parameter exists to offer — so equality was asserting a coincidence of machine timing, and contradicted the feature's own documented behaviour. Replaced with what the rule actually guarantees (finite score, loose bound); the parallel-path caveat is recorded in the test comment.
Validation
All 8 jobs green on run 30620073511, including the coverage job and the two macOS jobs that had never previously run:
No behaviour change: docs, wordlist, one comment tag, one test assertion.
🤖 Generated with Claude Code