Skip to content

nh-core: drop unnecessary tests - #763

Open
faukah wants to merge 2 commits into
masterfrom
faukah/push-ukpssqlklxwt
Open

nh-core: drop unnecessary tests#763
faukah wants to merge 2 commits into
masterfrom
faukah/push-ukpssqlklxwt

Conversation

@faukah

@faukah faukah commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

shlex has tests to test shlex, we can trust the shlex
tests to test shlex properly.

same with the other tests, we'll get a compiler error if these traits are not implemented.

Sanity Checking

  • I have read and understood the contribution guidelines
  • I have updated the changelog as per my changes
  • I have tested, and self-reviewed my code
  • Style and consistency
    • I ran nix fmt to format my Nix code
    • I ran cargo fmt to format my Rust code
    • I have added appropriate documentation to new code
    • My changes are consistent with the rest of the codebase
  • Correctness
    • I ran cargo clippy and fixed any new linter warnings.
  • If new changes are particularly complex:
    • My code includes comments in particularly complex areas to explain the
      logic
    • I have documented the motive for those changes in the PR body or commit
      description.
  • Tested on platform(s):
    • x86_64-linux
    • aarch64-linux
    • aarch64-darwin

Add a 👍 reaction to pull requests you find important.

faukah added 2 commits August 8, 2026 00:17
@faukah
nh-core: drop shlex tests
0f4514d 
shlex has tests to test shlex, we can trust the shlex
tests to test shlex properly.
@faukah
nh-core: drop trait tests
ab08d38 
the compailer will fail if these are traits are not implemented
anyways
@faukah faukah added the allow-no-changelog Doesen't need a CHANGELOG.md entry. label Aug 7, 2026
@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change removes obsolete command-line parsing and EnvAction behavior tests from crates/nh-core/src/command.rs. Production command execution and environment handling remain unchanged.

Changes

Command Test Cleanup

Layer / File(s) Summary
Remove command parsing tests
crates/nh-core/src/command.rs		 Removed tests for tokenization, quoting, escaping, environment variables, Nix paths, malformed quotes, sudo commands, and EnvAction trait behavior.

Suggested reviewers: notashelf

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely states that unnecessary nh-core tests were removed.
Description check ✅ Passed The description explains why redundant shlex and trait implementation tests were removed.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

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.

@NotAShelf

NotAShelf commented Aug 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

I think I considered dropping those much earlier, but decided against them because shlex tests might still fail without us knowing. If shlex tests fail, we don't realize they do, and get affected by a regression on an update we'll have no way to trace the error (unless we decide to look at shlex itself) and we'll have no early-indicators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

allow-no-changelog Doesen't need a CHANGELOG.md entry.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@faukah @NotAShelf