Skip to content

bash: Add tests.withChecks passthru to run tests#435033

Merged
infinisil merged 2 commits intoNixOS:masterfrom
tweag:bash-tests
Oct 10, 2025
Merged

bash: Add tests.withChecks passthru to run tests#435033
infinisil merged 2 commits intoNixOS:masterfrom
tweag:bash-tests

Conversation

@infinisil
Copy link
Member

@infinisil infinisil commented Aug 19, 2025

Allows running bash tests with

nix-build -A bash.tests.withChecks

This notably fails if a test fails, which is not a given with bash tests!

Note also that we can't run the tests by default (without other hacks) because the tests depend on a bunch of packages with bash as a dependency.

Note also that we had to disable some tests for now. In the future we could figure out how to make those work too.

CC @balsoft @trofi

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Aug 19, 2025
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. and removed 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Aug 19, 2025
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Aug 20, 2025
Copy link
Member

@balsoft balsoft left a comment

Choose a reason for hiding this comment

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

Actually, just noticed we missed a thing

+ echo $x
+ if ! sh $x; then
+ echo "Test $x failed"
+ #exit 1
Copy link
Member

Choose a reason for hiding this comment

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

Uncomment this before merging

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 23, 2025
@infinisil infinisil force-pushed the bash-tests branch 3 times, most recently from f924134 to 2a5b437 Compare August 26, 2025 17:03
@infinisil
Copy link
Member Author

Required some changes after the 5.3 update, works again now.

@trofi I added myself as a bash maintainer, would you also be okay with adding yourself? You've been doing a great job with maintenance :D. Since you don't have a maintainer entry yet, you'd need to create one in a separate PR though: https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix

@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 26, 2025
@trofi
Copy link
Contributor

trofi commented Aug 26, 2025

@trofi [...] would you also be okay with adding yourself?

No, I'll pass. I only have time for small drive-by changes.

Copy link
Member

@balsoft balsoft left a comment

Choose a reason for hiding this comment

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

Now looks good to me!

@infinisil
Copy link
Member Author

infinisil commented Sep 15, 2025

10 more tests were failing on Darwin, so I now just marked the test derivation as broken on Darwin because I don't want to bother trying to make them work or marking them as broken individually.

If we spend more time on this, let's focus on the Linux tests first.

I now also changed the test patch to collect all failures instead of just failing at the first one

@infinisil
Copy link
Member Author

infinisil commented Sep 30, 2025

@balsoft In addition to addressing your comment (and other improvements to the patch), I removed this:

# Fail if diff fails at the end of each test script
for check in tests/run-*; do
  # Append "exit $?" at the very end of the file
  sed -i '$aexit $?' "$check"
done

Everything works (or fails when expected) as it should, I guess that part isn't necessary after all. As a reminder, all the test files look like

${THIS_SH} ./arith.tests > ${BASH_TSTOUT} 2>&1
diff ${BASH_TSTOUT} arith.right && rm -f ${BASH_TSTOUT}

If the diff fails (aka there is a diff), the output doesn't get removed and it exits with 1. That exit code is already caught in the if sh $x; then, so nothing else should be needed.

@infinisil
Copy link
Member Author

@infinisil
Copy link
Member Author

I'll merge this, we can improve this over time if necessary

@infinisil infinisil added this pull request to the merge queue Oct 10, 2025
Merged via the queue into NixOS:master with commit b5cadfd Oct 10, 2025
29 of 31 checks passed
@infinisil infinisil deleted the bash-tests branch October 10, 2025 18:25
drupol added a commit to drupol/nixpkgs that referenced this pull request Nov 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants