refactor some tests using `subtest()` by aspiers · Pull Request #125 · aspiers/stow

aspiers · 2025-01-21T13:44:24Z

Summary by CodeRabbit

Tests
- Reorganized the test suite into focused subtests per scenario, converting long sequential flows into modular blocks with per-subtest planning.
- Moved setup/teardown into relevant subtests and reduced top-level test counts while preserving all assertions and expected outcomes.
- Improves readability, isolation, and maintainability without changing behavior.
Chores
- No user-facing changes; runtime behavior and public interfaces remain unchanged.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-10-07T23:29:38Z

Walkthrough

Refactors multiple test files to group assertions into scoped subtest blocks with per-subtest plans and moves related setup into those subtests; also removes an unused import from one test file. No behavioral changes to test outcomes or public/exported interfaces. (≤50 words)

Changes

Cohort / File(s)	Summary
CLI options tests `t/cli_options.t`	Rewrote a flat sequence of tests into several `subtest` blocks (basic CLI options, mixed options, deferred paths, overrides, ignored paths, no expansion). Each subtest declares its own plan and contains the original assertions; top-level test plan adjusted.
Examples tests `t/examples.t`	Broke a monolithic test into a setup `subtest` and several scenario `subtest` blocks; added per-subtest planning and isolated setup logic while preserving assertions and test intentions.
Foldable tests `t/foldable.t`	Converted inline tests into multiple focused `subtest` blocks (can fold a simple tree; can't fold an empty directory; can't fold if dir contains a non-link; can't fold if links point to different directories). Moved filesystem setup into subtests; assertions unchanged.
Minor cleanup in tests `t/unstow.t`	Removed an unused import (`make_path` from `File::Spec`); no other functional edits.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pay attention to test planning changes and top-level vs subtest plans (Test::More usage) in t/cli_options.t, t/examples.t, and t/foldable.t.
Verify filesystem setup/teardown scope moved into subtests to ensure no cross-test interference.
Confirm removal of make_path import in t/unstow.t doesn't affect any calls (search for its usage).

Poem

I'm a rabbit refactoring tests with care,
Nesting each case in a cozy lair.
Subtests snug like carrots in rows,
Assertions intact as the tidy code grows.
Hopping off now — may your CI lights glow fair. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main refactoring: reorganizing test files to use subtest() blocks for better structure.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch subtests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

t/examples.t (1)
27-27: LGTM! Excellent refactoring to subtests.

The test file is now properly organized into 5 meaningful subtests (setup, three stow scenarios, and bug fixes), with each subtest having its own test plan. The refactoring preserves all original test logic while improving clarity and failure isolation. This aligns perfectly with the coding guidelines requiring subtest organization.

Optional enhancement for better diagnostics:

The compound ok() assertions at lines 67-75, 94-105, and 121-151 could be split into multiple is() or ok() calls for more granular failure reporting. For example:
is($stow->get_conflict_count, 0, 'no conflicts');
ok(-l 'bin', 'bin is a symlink');
is(readlink('bin'), 'stow/perl/bin', 'bin points to correct location');
# ... and so on
This would make test failures easier to diagnose, but can be deferred to a separate improvement.

As per coding guidelines.

Also applies to: 35-198

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16a9dea and b9ffd09.

📒 Files selected for processing (3)

t/cli_options.t (1 hunks)
t/examples.t (1 hunks)
t/foldable.t (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

t/**/*.t

📄 CodeRabbit inference engine (AGENTS.md)

t/**/*.t: Write tests under t/ using Test::More and Test::Output
Each test should use t::testutil (import testutil.pm) for shared helpers
Initialize a clean test environment by calling init_test_dirs() at the start of tests
Organize assertions using subtest('description', sub { ... }) and split files into meaningful subtests

Files:

t/foldable.t
t/examples.t
t/cli_options.t

🧠 Learnings (2)

📓 Common learnings

Learnt from: CR
PR: aspiers/stow#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T23:25:46.639Z
Learning: Applies to t/**/*.t : Organize assertions using `subtest('description', sub { ... })` and split files into meaningful subtests

📚 Learning: 2025-10-07T23:25:46.639Z

Learnt from: CR
PR: aspiers/stow#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T23:25:46.639Z
Learning: Applies to t/**/*.t : Organize assertions using `subtest('description', sub { ... })` and split files into meaningful subtests

Applied to files:

t/foldable.t
t/examples.t
t/cli_options.t

🔇 Additional comments (2)

t/cli_options.t (1)

25-25: LGTM! Clean subtest organization.

The CLI option tests are now properly organized into 6 focused subtests, each testing a specific aspect of command-line option processing. The consistent pattern (set @argv → process_options() → assert) makes the tests easy to understand and maintain. Each subtest correctly uses local @ARGV to scope changes and has an appropriate test plan.

As per coding guidelines.

Also applies to: 33-119

t/foldable.t (1)

27-27: LGTM! Excellent test organization.

The foldable tests are now cleanly organized into 4 subtests, each testing a distinct scenario with proper setup and teardown isolation. The use of is() assertions (rather than compound ok() conditions) provides clear failure diagnostics. Each subtest appropriately scopes its setup code and has a focused test plan.

As per coding guidelines.

Also applies to: 35-80

aspiers · 2025-10-08T08:58:01Z

@coderabbitai Did I accidentally remove any tests?

coderabbitai · 2025-10-08T08:59:27Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Get the diff for each test file and analyze test count changes
echo "=== Analyzing t/cli_options.t ==="
git diff master...HEAD -- t/cli_options.t | grep -E '^\+.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l
echo "Added test assertions: $(git diff master...HEAD -- t/cli_options.t | grep -E '^\+.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"
git diff master...HEAD -- t/cli_options.t | grep -E '^-.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l
echo "Removed test assertions: $(git diff master...HEAD -- t/cli_options.t | grep -E '^-.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"

echo ""
echo "=== Analyzing t/examples.t ==="
echo "Added test assertions: $(git diff master...HEAD -- t/examples.t | grep -E '^\+.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"
echo "Removed test assertions: $(git diff master...HEAD -- t/examples.t | grep -E '^-.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"

echo ""
echo "=== Analyzing t/foldable.t ==="
echo "Added test assertions: $(git diff master...HEAD -- t/foldable.t | grep -E '^\+.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"
echo "Removed test assertions: $(git diff master...HEAD -- t/foldable.t | grep -E '^-.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"

echo ""
echo "=== Analyzing t/unstow.t ==="
echo "Added test assertions: $(git diff master...HEAD -- t/unstow.t | grep -E '^\+.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"
echo "Removed test assertions: $(git diff master...HEAD -- t/unstow.t | grep -E '^-.*\b(ok|is|is_deeply|like|isnt|cmp_ok)\(' | wc -l)"

echo ""
echo "=== Detailed diff analysis for each file ==="
echo "--- t/cli_options.t ---"
git diff master...HEAD -- t/cli_options.t | grep -E '^[-+].*\b(ok|is|is_deeply|like|isnt|cmp_ok)\('

echo ""
echo "--- t/examples.t ---"
git diff master...HEAD -- t/examples.t | grep -E '^[-+].*\b(ok|is|is_deeply|like|isnt|cmp_ok)\('

echo ""
echo "--- t/foldable.t ---"
git diff master...HEAD -- t/foldable.t | grep -E '^[-+].*\b(ok|is|is_deeply|like|isnt|cmp_ok)\('

Length of output: 3532

@aspiers No, you didn't accidentally remove any tests! Here's what I found:

✅ t/cli_options.t: All 10 assertions preserved (just reindented into subtests)
✅ t/examples.t: Actually added 1 assertion - the new ok(1, 'created fake packages') in the setup subtest (went from 10 to 11 total assertions)
✅ t/foldable.t: All 4 assertions preserved (just reindented into subtests)
✅ t/unstow.t: No changes to assertions (only removed an unused import)

The plan count reductions you see (10→6 in cli_options.t, 10→5 in examples.t) are because subtests are now counted as single tests in the top-level plan, but all the individual assertions within those subtests remain intact. This is the expected behavior when refactoring to use subtest() blocks.

🧠 Learnings used

Learnt from: CR
PR: aspiers/stow#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T23:25:46.639Z
Learning: Applies to t/**/*.t : Organize assertions using `subtest('description', sub { ... })` and split files into meaningful subtests

coderabbitai