Skip to content

Fix expand tests and harden the scheduled workflows - #87

Merged
madonoharu merged 8 commits into
mainfrom
fix/expandtest-features
Aug 12, 2026
Merged

Fix expand tests and harden the scheduled workflows#87
madonoharu merged 8 commits into
mainfrom
fix/expandtest-features

Conversation

@madonoharu

@madonoharu madonoharu commented Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Fixes the expand tests (3 of 4 failing in CI since June) and hardens the scheduled workflows so this class of breakage can't go unnoticed again.

Root cause

macrotest translates tsify's [features] table into its temporary test project but drops feature references that don't start with dep:. --features tsify/json therefore never enabled the temp project's own serde / wasm-bindgen optional deps, so the code emitted by #[derive(Tsify)] couldn't resolve those crates (E0463/E0433). Older rustc tolerated unresolved macros during expansion and printed partially-expanded output — that's what the old snapshots recorded. Current rustc makes this a hard error. Nothing in this repo changed; the toolchain did.

Changes

  • expandtest: pass --features json,wasm-bindgen (the test project's own features) and regenerate the snapshots. They are now fully expanded, which also restores the diff sensitivity the nightly cron relies on — the old partially-expanded snapshots never changed across 13 wasm-bindgen releases, which is why the cron never opened an update PR.
  • Snapshots track the latest wasm-bindgen (Cargo.lock is gitignored; CI resolves fresh), currently 0.2.127. Future codegen changes will be picked up by the cron as designed.
  • #[tsify::declare] unit tests: with full expansion, type_alias.expanded.rs no longer captures the emitted TS declaration (typescript_custom_section expands to nothing on non-wasm targets), so tsify-macros now asserts it directly.
  • check-wasmbindgen-changes.yml: explicit token permissions, resolved wasm-bindgen version in the logs and PR title, a tracking issue on failure (exact-title deduped, auto-closed on recovery), assignee/label on auto-PRs, and PR-creation failures now propagate instead of being swallowed.
  • latest-deps.yml (new): weekly full test run against fresh dependency resolution, with the snapshot comparison excluded from pass/fail — routine codegen drift belongs to the nightly cron.
  • Shared composite actions (new, .github/actions/): both scheduled workflows use the same environment setup and tracking-issue plumbing. Setup now calls rustup directly instead of the archived actions-rs/toolchain.
  • CONTRIBUTING.md (new): test setup, snapshot regeneration (fresh lockfile first), and the lockfile policy.

Note

The cron's PR-creation path has never fired in production; worth a manual workflow_dispatch run after this merges to prove it end-to-end.

🤖 Generated with Claude Code

@madonoharu
madonoharu marked this pull request as draft August 11, 2026 21:13
@madonoharu madonoharu self-assigned this Aug 11, 2026
@madonoharu @claude
Fix expandtest: enable the test project's own features for expansion
7f87e15 
The expandtest has been failing (3 of 4 tests) both locally and in CI
because `--features tsify/json` only enables tsify's features in the
temporary project macrotest generates, while that project's own optional
`serde` / `wasm-bindgen` dependencies stay disabled (macrotest's feature
translation drops non-`dep:` feature references). The code emitted by
`#[derive(Tsify)]` then fails to resolve `serde` / `wasm_bindgen`
(E0463 / E0433). Older rustc tolerated unresolved macros during
`-Zunpretty=expanded` and printed partially-expanded output; current
rustc makes this a hard error, which is why CI has been red with no
repo change.

- Pass `json,wasm-bindgen` so the test project's own features (and thus
  `dep:serde` / `dep:wasm-bindgen`) are enabled
- Regenerate expected outputs with `MACROTEST=overwrite`; wasm-bindgen
  attribute macros are now fully expanded instead of being left
  unexpanded as in the previous partially-expanded form. Since
  Cargo.lock is gitignored and CI resolves dependencies fresh, the
  outputs are generated against the current latest wasm-bindgen
  (0.2.127) to match what CI expands.

Verified with wasm-bindgen 0.2.127: ./test.sh (cargo test --all, -F js,
wasm-pack test --node x2, e2e build + compare), cargo fmt --all --
--check, and cargo clippy -- -D warnings all pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu
madonoharu force-pushed the fix/expandtest-features branch from e9f8514 to 7f87e15 Compare August 11, 2026 22:06
@siefkenj

siefkenj commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Thanks for noticing this. The github cronjob was supposed to fix this issue once and for all but I guess it didn't...

Would you be able to update the cron job so that it correctly makes PRs whenever the expand tests start failing?

madonoharu and others added 3 commits August 12, 2026 08:10
@madonoharu @claude
Harden scheduled CI: fail-loud notifications and latest-deps job
f1dbf10 
The nightly expand-check cron has never produced an update PR since its
introduction (109 green runs during which 13 wasm-bindgen releases went
undetected because the committed snapshots had degraded to a partially
expanded form, then 75 consecutive failures that notified no one).

- check-wasmbindgen-changes.yml:
  - add explicit token permissions (contents/pull-requests/issues);
    without them PR/issue creation can fail silently depending on
    repository settings
  - log the freshly resolved wasm-bindgen version and rustc version to
    the step summary, and use the version in the update PR title
  - open a tracking issue (once per breakage) when the job fails,
    instead of failing silently in the Actions tab
  - assign and label the auto-created update PR; tolerate an existing PR
- latest-deps.yml (new): weekly + manual full ./test.sh against the
  newest compatible dependency resolution, without snapshot overwrite —
  the configuration downstream users actually get; previously this was
  never exercised anywhere
- CONTRIBUTING.md (new): document the snapshot regeneration procedure
  (fresh lockfile first — the stale-lockfile trap behind PR #87's first
  CI failure) and the deliberate no-committed-lockfile policy
- .gitignore: turn the Cargo.lock entry into an explicit, documented
  decision

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu @claude
Address adversarial review findings (2x Opus, Codex, Claude)
77d311a 
- tsify-macros: add unit tests for the TS declaration emitted by
  `#[tsify::declare]`. The fully-expanded snapshots can no longer see it:
  `#[wasm_bindgen(typescript_custom_section)]` expands to nothing on
  non-wasm targets, which had silently reduced type_alias.expanded.rs to
  an empty proof (a sentinel mutation in the macro passed the test).
  [found independently by 3 of 4 reviewers]
- check-wasmbindgen-changes.yml: replace the `gh pr create || echo`
  catch-all with an explicit existing-PR check; create the PR bare and
  apply assignee/label afterwards so decoration failures cannot suppress
  it. The old pattern reported "PR already exists" for *any* failure
  (403, missing label, ...) and turned the job green. [consensus P1]
- both workflows: auto-close the failure tracking issue on the next
  successful run — an issue left open would mute all future alerts,
  recreating the very silence this PR is meant to fix.
- latest-deps.yml: exclude the expandtest snapshot comparison from the
  pass/fail signal (`--skip expandtest` + an informational step).
  Routine wasm-bindgen codegen drift belongs to the nightly cron;
  without this, every release would abort ./test.sh at the first
  command and file a false "compatibility break" issue.
- CONTRIBUTING.md: state the lockfile policy factually (the previous
  wording implied a documented maintainer decision that does not exist
  in-repo).

Verified: cargo test --all (incl. new unit tests and expandtest), fmt,
clippy -D warnings, YAML syntax, and --skip expandtest filtering.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu @claude
Polish wording and simplify implementation (3-way refactor pass)
d5fe61e 
Applied the merged recommendations of three independent refactoring
passes (two Opus reviewers and Codex):

- workflows: hoist the tracking-issue title into a job-level env var
  (single source of truth — a title mismatch between the notify and
  close steps would permanently mute alerts), flatten "Check for
  changes" into guard clauses, tighten comments
- CONTRIBUTING.md / .gitignore: tighter, more natural wording; same
  facts and warnings
- type_alias.test.rs: assert_contains! macro in the style of
  ts_type.test.rs; more precise header comment
- expandtest.rs: move the rationale comment to a //! doc comment

No behavioral change except one deliberate addition: the cron now logs
"nothing to do" when snapshots are unchanged (previously silent).
All four review-hardening guarantees re-verified: pr-create failure
propagation, decoration isolated from PR creation, issue dedup +
auto-close, expandtest excluded from latest-deps pass/fail.

Verified: YAML syntax, bash -n on all run blocks, cargo test --all,
fmt --check, clippy -D warnings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu madonoharu changed the title Fix expandtest by enabling the test project's own features Fix expand tests and harden the scheduled workflows Aug 12, 2026
@madonoharu
madonoharu marked this pull request as ready for review August 12, 2026 00:30
@madonoharu
madonoharu requested a review from siefkenj August 12, 2026 00:34
madonoharu and others added 4 commits August 12, 2026 09:38
@madonoharu @claude
Use cargo pkgid instead of grepping Cargo.lock for the resolved version
0ed8683 
`cargo pkgid wasm-bindgen` is the purpose-built way to read the resolved
version: no dependence on the lockfile's TOML layout, no external
commands (bash parameter expansion strips the `name@` prefix), and it
errors out loudly if multiple versions were ever resolved instead of
silently picking the first match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu @claude
Extract shared workflow steps into composite actions
2f617c5 
The two scheduled workflows duplicated their entire environment setup
(Node, toolchain, wasm-pack, cargo-expand, fresh dependency resolution
+ version logging) and the tracking-issue plumbing (dedup search, open
on failure, close on success). Extract both into local composite
actions:

- .github/actions/setup-test-env: setup + fresh lockfile, exposing the
  resolved wasm-bindgen version as an output
- .github/actions/track-failure-issue: mode: notify | close, with the
  issue title passed in from the caller so both modes keep searching
  for the same string

Workflow-specific content (issue bodies, job comments, ISSUE_TITLE)
stays in the workflows. As a side effect, issue bodies moved from
inline shell strings to `with:` block scalars, which removes the
leading indentation that previously leaked into the rendered issues.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu @claude
Align composite action descriptions with the established wording
cc4edfe 
The new action docs drifted from phrasing the earlier editing pass had
settled on: "what downstream users actually get" (not "the
configuration ..."), "resolves fresh" (not "pins ... to the latest" —
pinning means the opposite), and the mute-warning phrased as "one left
open would mute every later breakage" to match the workflow comments.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu @claude
More purpose-built flags over generic shell (second-opinion pass)
6e0b03f 
Discussed with an independent model which generic-shell constructs
still have purpose-built equivalents, verified each locally:

- `gh label create --force` replaces `2>/dev/null || true` — updates an
  existing label instead of erroring, and a real failure now surfaces
  as a workflow warning instead of vanishing
- exact-title match for the tracking-issue lookup: GitHub's `in:title`
  search matches substrings, so filter with `--jq 'map(select(.title ==
  env.ISSUE_TITLE))'` (gh's gojq exposes the environment) — a title
  that is a substring of another issue's can no longer mute alerts
- install the toolchain with rustup directly: the archived
  actions-rs/toolchain action (node12) only wrapped the same rustup
  call the runner already ships
- fold the two step-summary writes into one `tee`

Considered and rejected with evidence: `gh pr view <branch>` for the
existing-PR check (returns closed/merged PRs when no open one exists),
and replacing the pinned wasm-pack action (its `latest` still resolves
to 0.9.1, and `cargo install` trades a seconds-fast binary download for
a minutes-long source build).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madonoharu
madonoharu merged commit f5bb30d into main Aug 12, 2026
3 checks passed
@madonoharu
madonoharu deleted the fix/expandtest-features branch August 12, 2026 09:45
This was referenced Aug 17, 2026
Merged
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siefkenj siefkenj Awaiting requested review from siefkenj

Assignees

@madonoharu madonoharu

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@madonoharu @siefkenj