refactor(test): prefer cfg_attr(ignore) over cfg for conditional test skipping#369
Merged
Merged
Conversation
…test skipping Use `#[cfg_attr(..., ignore = "reason")]` instead of `#[cfg(not(...))]` on tests that can still compile under the skipped condition. This keeps the tests compiled on all configurations, catching type errors early while only skipping execution at runtime. Changes: - cli_errors::fs_errors: replace `#[cfg(not(pdu_test_skip_fs_errors))]` with `#[cfg_attr(pdu_test_skip_fs_errors, ignore)]`; remove the same gate from its imports and helper function - test_remove_overlapping_paths: remove `#[cfg(unix)]` from module declaration; add `#[cfg_attr(not(unix), ignore)]` to each test - Add "Conditional Test Skipping" section to CONTRIBUTING.md - Add guideline bullet to AI instruction templates https://claude.ai/code/session_0184ceZayyCwvK7PEF9iB7K3
- Change ignore reason to "only one path separator style is tested" - Reorder attributes: place #[cfg_attr(..., ignore)] below #[test] - Revert hint message in fs_errors back to "skip" - Soften "Always" to "Prefer" for reason strings in CONTRIBUTING.md - Rephrase CONTRIBUTING.md to mention tests are "still compiled" https://claude.ai/code/session_0184ceZayyCwvK7PEF9iB7K3
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors conditional test skipping to prefer #[cfg_attr(..., ignore = "...")] over #[cfg(...)], so tests still compile across configurations/platforms while being skipped at runtime when appropriate.
Changes:
- Documented the recommended patterns for conditional test skipping in
CONTRIBUTING.md(with good/bad examples). - Updated unit/integration tests to compile unconditionally where possible, switching from
#[cfg(not(...))]to#[cfg_attr(..., ignore = "...")]. - Propagated the new guideline into the AI instruction templates and generated instruction files.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cli_errors.rs | Converts fs_errors to cfg_attr(..., ignore) and removes now-unnecessary cfg(not(...)) gating around supporting imports/helpers. |
| src/app/overlapping_arguments.rs | Removes #[cfg(unix)] from the test module so it compiles on all platforms under cfg(test). |
| src/app/overlapping_arguments/test_remove_overlapping_paths.rs | Adds cfg_attr(not(unix), ignore = "...") to Unix-path-style tests to keep them compiling everywhere but skipped where not applicable. |
| CONTRIBUTING.md | Adds a dedicated section clarifying when to use #[cfg] vs #[cfg_attr(..., ignore)] for tests. |
| template/ai-instructions/shared.md | Adds the new conditional test skipping guideline to the shared AI-instructions template. |
| CLAUDE.md | Updates generated AI instructions to include the new guideline. |
| AGENTS.md | Updates generated AI instructions to include the new guideline. |
| .github/copilot-instructions.md | Updates generated AI instructions to include the new guideline. |
Performance Regression Reportscommit: 2a5a78e There are no regressions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR updates the codebase to follow a better practice for conditionally skipping tests: using
#[cfg_attr(..., ignore)]instead of#[cfg(...)]to ensure tests compile on all platforms while being skipped at runtime when conditions aren't met.Key Changes
CONTRIBUTING.mdexplaining when to use#[cfg_attr(..., ignore)]vs#[cfg(...)]for tests, with clear examples of good and bad patterns#[cfg(unix)]from thetest_remove_overlapping_pathsmodule declaration insrc/app/overlapping_arguments.rsso the module compiles on all platforms#[cfg_attr(not(unix), ignore = "only one path separator style is tested")]to 7 tests insrc/app/overlapping_arguments/test_remove_overlapping_paths.rsthat use hardcoded Unix paths but don't reference Unix-only typesfs_errorstest intests/cli_errors.rsfrom#[cfg(not(pdu_test_skip_fs_errors))]to#[cfg_attr(pdu_test_skip_fs_errors, ignore = "...")], allowing it to compile in all configurations#[cfg(not(pdu_test_skip_fs_errors))]guards from imports and helper functions intests/cli_errors.rsthat are now always availableImplementation Details
The key principle: tests should compile everywhere to catch type errors and regressions early, but skip at runtime when conditions aren't met. Use
#[cfg]on tests only when the code literally cannot compile under certain conditions (e.g., references platform-specific types that don't exist).All changes maintain backward compatibility while improving test coverage visibility across all platforms.
https://claude.ai/code/session_0184ceZayyCwvK7PEF9iB7K3