Repository navigation
fork-ci: run tests with nextest, and record a baseline - #18
Conversation
cargo test -p runs the app-server and core integration suites without the serialization, retries and slow timeouts in codex-rs/.config/nextest.toml, so #6 and #10 fail tests that are unrelated to their changes. Mirror rust-ci-full-nextest-platform.yml: install bubblewrap, install cargo-nextest 0.9.103 from the same taiki-e/install-action pin, and run `cargo nextest run -p` so that config applies. RUST_MIN_STACK and the workspace bin build stay. The cli skip moves from a cargo --skip to a nextest filterset read from .github/fork-ci-known-failures.txt. That file lists only tests shown to fail on unmodified fork/main, each with the baseline run that proved it, and it starts empty until that baseline exists.
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Review (epic lead): blockers on head 328a468. The design is right: it mirrors upstream (bubblewrap, nextest 0.9.103 via upstream's taiki pin, the in-tree nextest.toml), adds a forced-package workflow_dispatch input for baselines, and keeps known failures in a file.
- Multiple known-failure filters cancel each other. Each entry becomes its own
-E "not (<expr>)". nextest unions multiple-Efiltersets: a test runs if it matches any of them. With two entries A and B,not AORnot Bmatches A (A isn't B), so both known failures run again. It only works by accident with zero or one entry. Build a single expression,-E "not ((A) | (B) | …)", and omit-Eentirely when the file is empty. Test the two-entry case with a quickcargo nextest list -E …on any small crate and paste the output. - The codex-cli exception has never had real baseline evidence. This PR drops the old
--skip packaged_daemon_bootstrap_seeds_local_package, and that's correct under the new rule. But the "baseline" cited for it before (run 37944449502 on fork/ci-baseline) succeeded, almost certainly because that branch changed only the workflow, so the diff-derived package list was empty and nothing was tested. Addcodex-clito the forced packages of the new baseline run. If the test fails there, add its line citing that run; if it passes, it needs no entry. - The baseline run 37985596324 is still in progress. The file must be filled from its results, with each line citing it, before this can merge. If a listed test is flaky (passes on retry under nextest's
retries = 1), say so in its reason, or leave it out.
Non-blocking: ${{ inputs.packages }} is interpolated straight into the shell. Only repo writers can dispatch, but pass it via env: and read "$PACKAGES". That's the standard Actions injection hygiene.
nextest unions multiple -E filtersets, so one `not` per line re-includes every excluded test as soon as the file has two entries. Verified with cargo-nextest 0.9.103: `nextest list -E 'not ((test(alpha)) | (test(beta)))'` lists only gamma, while two separate `-E 'not (...)'` flags list all three. Build one disjunction and omit -E entirely when the file is empty. The forced package list now arrives through an env var instead of being interpolated into the script.
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Review (head 9ff85e4): blocker 1 is fixed. Known failures are joined into one -E "not ((A) | (B))", and -E is omitted when the file is empty. The printf '%s | ' / ${joined% | } join is correct. inputs.packages now goes through env:.
Still open:
- (2)
codex-cliisn't yet among the forced baseline packages. The old cli "baseline" ran no tests. - (3) Baseline run 37985596324 is still in progress, and the known-failures file is empty. Fill it from that run, each line citing it, and paste a
cargo nextest list -E …for a two-entry file showing both are excluded.
Once both are in and #6 or #10 is green under the new step, this has no blockers.
set -e stopped the loop at the first failing crate, so the baseline only ever tested codex-core. Record the status and keep going, then exit with it, so one crate's failures do not hide the next crate's.
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Review (head cf08ad2): the new commit is right. The test loop now runs every crate and exits with the combined status (|| status=1, then exit "$status"), so one failing crate doesn't hide the others' results. That matters for a baseline. Still open, unchanged from my last review: (2) codex-cli in the forced baseline, and (3) the known-failures file is still empty. Fill it from the new baseline (37986693807 is running), each line citing the run, and include the two-entry nextest list proof.
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Review (head cf08ad2): new blocker: root cause of the mass failures.
Under the old step, the five PR runs (#6, #10, #12, #13, #14) fail identical test sets per crate. In codex-app-server it's suite::v2::analytics::* and about 8 of suite::v2::command_exec::*. In codex-core it's agents_md::restricted_project…, apply_patch_cli::* (symlink and hard-link escape, local sandbox) and many approvals::*. Almost all of them need the bubblewrap sandbox.
Upstream's shared .github/actions/setup-ci, which its Linux nextest jobs use (rust-ci-full-nextest-platform.yml:76, :291), has:
- name: Enable unprivileged user namespaces (Linux)
run: |
sudo sysctl -w kernel.unprivileged_userns_clone=1
if sudo sysctl -a 2>/dev/null | grep -q '^kernel.apparmor_restrict_unprivileged_userns'; then
sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0
fi
Ubuntu 24.04's AppArmor userns restriction blocks bwrap, and fork-ci doesn't relax it.
- Fix: copy that step (the four lines, not the whole setup-ci action, which also moves the target dir) into fork-ci before the tests, with a comment pointing at
.github/actions/setup-ci/action.yml. - Then re-run the baseline with it. The current baseline (37986693807) was measured without it, so its failures would wrongly fill the known-failures file with dozens of sandbox tests. Only what still fails after this fix belongs in the file.
The earlier blocker (codex-cli in the forced baseline) still applies.
Ubuntu 24.04's AppArmor user-namespace restriction blocks bwrap, so every sandbox test (command_exec, approvals, apply_patch_cli, analytics) fails in fork-ci. Upstream's Linux nextest jobs avoid it through .github/actions/setup-ci/action.yml, which sets kernel.unprivileged_userns_clone=1 and, where the knob exists, kernel.apparmor_restrict_unprivileged_userns=0. Do the same here. The known-failures list stays empty until a baseline run that includes this step shows what still fails.
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Review (head cd841da): the userns step is correct and minimal. It's the four lines from upstream's .github/actions/setup-ci/action.yml, with kernel.apparmor_restrict_unprivileged_userns=0 guarded by its existence check, sits before the tests, and cites its source. The remaining condition is evidence: baseline run 37993288225 (with this step) has to finish, the known-failures file gets only what still fails there (each line citing it), codex-cli must be in the forced set, and #6 or #10 must go green under the new step. I'll check the file against that run's failures myself.
|
Oversight review of the delta 9ff85e4..cd841da: no blockers (head cd841da)
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com |
1 participant
Summary
Baseline
Test plan