Skip to content

fix(test): strip inline tests from local-extended children of remote presets#339

Merged
fohte merged 4 commits into
mainfrom
fohte/fix-strip-nested-preset-tests
May 4, 2026
Merged

fix(test): strip inline tests from local-extended children of remote presets#339
fohte merged 4 commits into
mainfrom
fohte/fix-strip-nested-preset-tests

Conversation

@fohte
Copy link
Copy Markdown
Owner

@fohte fohte commented May 4, 2026

Purpose

  • runok test evaluates a large number of preset-authored inline tests regardless of any user override, and a user override turns those into expected allow, got deny failures
    • Example: extending github:fohte/runok-presets/base and denying find * in the user config produces a flood of failures such as find . -name "*.txt" => expected allow, got deny

Reproduction steps

  1. Write the following to /tmp/test-strip.yml
    extends:
      - "github:fohte/runok-presets/base"
    rules: []
  2. Running runok test -c /tmp/test-strip.yml evaluates the preset's own inline tests in bulk, e.g. 592 passed, 0 failed, 592 total

Approach

  • Once an extends chain crosses into any remote preset, treat every descendant as preset-authored and strip its inline tests and top-level tests: block
    • The strip introduced in #227 decided based on the loaded reference's own kind, so a remote preset like runok-presets/base whose extends: [./readonly-unix.yml, ...] reaches its siblings via local paths left those siblings outside the strip
  • Tests in the user's own config and in presets the user extends directly via a local path are preserved as before
Design decisions

Where to apply the strip

Decision Design Pros Cons
Chosen Keep the existing strip in load_preset_with and additionally strip in resolve_extends_recursive while propagating an under_remote flag down the chain Direct callers of load_preset that skip extends resolution (such as the version check in update-presets) keep stripping the remote root unchanged The strip lives in two places, so future strip targets must be added through strip_preset_tests so both call sites stay in sync
Rejected Move the strip out of load_preset_with and centralise it in resolve_extends_recursive The strip has a single responsibility Direct callers of load_preset that skip extends resolution would stop stripping the remote root, changing existing behaviour

fohte added 3 commits May 4, 2026 18:19
intent(test): prevent preset-authored tests from being evaluated under
  downstream user overrides when a remote preset reaches its child files
  via local-path extends (the layout used by runok-presets/base, which
  extends ./readonly-unix.yml etc.). Previously only the outermost remote
  file was stripped; nested children leaked their tests and failed under
  user overrides such as deny: 'find *'.
decision(test): track an under_remote flag through resolve_extends_recursive
  so any preset reached via a remote ancestor is stripped, regardless of
  whether each child reference itself is remote or local. The user's own
  config and presets they extend directly via local paths keep their tests.
rejected(test): stripping based purely on PresetReference::Local at load
  time — the load site cannot see whether the parent in the chain was
  remote, so it cannot decide on its own.
intent(test): close a gap raised in pre-push review — verify that crossing
  the remote boundary partway through an extends chain (user → local → remote
  → local-under-remote) only strips presets at and after the boundary. The
  previously added tests covered remote-at-the-root and local-only chains
  but not this mixed shape, which is what real downstream configs look like.
decision(test): also pin the invariant — `load_preset_with` and
  `resolve_extends_recursive` apply the same strip in two places (the former
  for direct callers like `load_preset`, the latter for nested children
  reached via local paths). The double pass is idempotent but easy to
  retrofit incorrectly, so leave a comment requiring future strip targets to
  be added in `strip_preset_tests` rather than only at the load site.
intent(docs): satisfy the project rule that release notes entries must
  carry their PR number; the placeholder existed because the PR was not
  yet open at draft time.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

❌ Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.89%. Comparing base (1feddf1) to head (c629873).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/config/preset.rs 95.74% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
+ Coverage   88.78%   88.89%   +0.11%     
==========================================
  Files          53       53              
  Lines       12322    12369      +47     
==========================================
+ Hits        10940    10996      +56     
+ Misses       1382     1373       -9     
Flag Coverage Δ
Linux 88.72% <95.74%> (+0.13%) ⬆️
macOS 90.04% <95.74%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes a bug where runok test would evaluate inline tests from presets reached via a remote ancestor, even if those presets were referenced through local paths. The fix involves tracking the remote status through the recursive extension resolution and stripping tests from any preset loaded under a remote boundary. A review comment correctly identifies that the new tests repeat setup logic for PresetCache and CacheMetadata, which should be extracted into a #[fixture] according to the repository's style guide to improve maintainability.

Comment thread src/config/preset.rs Outdated
intent(preset): the four preset-strip tests duplicated the same PresetCache
  and CacheMetadata setup, which the project test guide says should live
  in a fixture or helper. Moving the construction into `make_cache` and
  `seed_remote_preset` makes the failure-relevant differences (which preset
  files each scenario seeds) the only thing each test spells out.
@fohte fohte merged commit 63b6830 into main May 4, 2026
10 checks passed
@fohte fohte deleted the fohte/fix-strip-nested-preset-tests branch May 4, 2026 09:43
@fohte-bot fohte-bot Bot mentioned this pull request May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant