Repository navigation
build: run ignored tests in ci and delete xmutil - #1080
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1080 +/- ##
========================================
Coverage 92.73% 92.74%
========================================
Files 311 310 -1
Lines 191075 190939 -136
========================================
- Hits 177200 177081 -119
+ Misses 13875 13858 -17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f346e834ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // cannot carry), and it is the rule the MDL equivalence harness | ||
| // already applies to compare us with xmutil, which emits | ||
| // apply-to-all here. | ||
| // cannot carry), and it is what xmutil emits here. |
There was a problem hiding this comment.
After this change deletes both the xmutil equivalence harness and the vendored xmutil source, this sentence becomes an unsupported external-tool assertion beside the importer decision. The remaining corpus tests establish numerical parity with Vensim, not that xmutil emits ApplyToAll, so cite the upstream xmutil function/file and line that proves this representation or mark/remove the assertion.
AGENTS.md reference: AGENTS.md:L104-L110
Useful? React with 👍 / 👎.
Ignored engine tests ran nowhere, so a pin could go stale unnoticed (the C-LEARN LTM digest has been). A gates cargo profile, scripts/gates.sh and a CI job run every ignored test optimized, with debug assertions on, on every push to main and pull request to main; saying what the test sweeps. The script finds ignored tests itself, so a new gate needs no list entry. The stale digest is skipped by name until its re-pin lands. xmutil goes with its comparison tests: the importer is held to Vensim's own output, and it was the only C++ in the build. Several checks could not fail and now can: test-asan.sh lost its exit status to tee and ran with halt_on_error=0, and the pysimlin tag guard skipped whenever CI fetched no tags. CI also pins its runner image, lints what the hook lints, moves to the Node 24 action majors (setup-uv pinned to a release, since it publishes no major tags), and reports advisories from a scheduled workflow instead of failing unrelated pull requests.
f346e83 to
337a34a
Compare
Why
A fresh-eyes review of the build and CI found checks that could not fail and tests that ran nowhere.
teewithoutpipefailand ran withhalt_on_error=0, so neither a failing test nor a leak changed its exit status.cargo deny check advisoriesin the PR lint job turned unrelated work red whenever an advisory was published overnight.test/.What
gatescargo profile (release without LTO, with debug assertions and overflow checks);scripts/gates.shruns the engine's lib and integration suites with--include-ignored,ext_dataand--no-fail-fast; agatesCI job calls it. The script finds ignored tests itself, so a new gate needs no list entry. Every#[ignore]in the engine reads"<what it sweeps>; run under the gates profile", anddocs/dev/rust.mdstates the rules.src/xmutil, thexmutilfeature,open_vensim_xmutil,mdl_equivalence.rs,test_open_vensim.rs, and their dependency-policy, CI and doc references.test/xmutil_test_models/(test data other tests use) stays.test-asan.sh:pipefail,--no-fail-fast,halt_on_errorat its default, andallocator_may_return_null=1.--all-targets --all-features;cargo deny check bans licenses sourcesin the PR gate and advisories in their ownadvisories.yml(daily, plus lockfile changes; files an issue on a scheduled failure); dependabot for actions; the Node 24 action majors;ubuntu-24.04pinned.test-generatordev-dependency,UnitMap.ctx, a leak intest_common.rs; an interned-slot-reuse test (one default-suite row and one gate).Decisions worth a reviewer's eye
simulate_ltm::clearn_ltm_slot_maxima_digestis skipped by exact name ingates.sh, with a comment, so the job is not red on its first day. The loop-analysis change re-pins it and deletes the skip.astral-sh/setup-uvis pinned tov10.2.0. That action stopped publishing moving major tags at v8, so@v10does not resolve. An adversarial review caught this; it would have failed three jobs at "Set up job" on merge.allocator_may_return_null=1under ASan: the engine refuses an impossible run throughtry_reserve, and ASan's default aborts the process where the allocator would return an error. An infallible allocation that gets null still aborts.Evidence
scripts/gates.sh: the default suite shows 33 ignored engine tests and the gates run shows none (6,542 lib and 873 integration tests pass; one filtered out, the digest). About 80 s to run on this machine, 111 s pinned to four cores, after a cold build of about 105 s. A planted failing gate and a planted compile error each make it exit non-zero../test-asan.sh: exits 0 unmodified; with a planted failing test and a planted 4 KB leak it exits non-zero and reports each. Before the allocator option it failed on main on a test that expects an allocation refusal.GITHUB_ACTIONSwithout them; CI's fetch line brings the tags in a fresh depth-1 clone.uses:ref in.github/(22 distinct) resolves withgit ls-remote. The reviewer read each bumped action's release notes for breaking changes that touch how this repo uses it and found none.cargo deny(all four checks), the dependency, docs and workflow-path checks, and the full pre-commit hook pass.What this does not establish
No workflow has run on GitHub with these changes. Known only on the first runs:
pnpm/action-setupv6 updating itself from its pnpm 11 bootstrap to the pinned 10.6.0;setup-uvv10.2.0's cache behavior, including that v10 disables caching onreleaseevents;actions/checkoutv7 writing credentials to a separate file, under cibuildwheel's containers at release time;Also: the digest is skipped here, not re-derived; and no workflow linter was available, so the workflows were checked by parsing and reading.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LYR7N9WZwQuFnZFW8JDUgx