Skip to content

tests.overriding: refactor and allow non-bool expected value#456848

Merged
MattSturgeon merged 2 commits intoNixOS:masterfrom
ShamrockLee:test-overriding-refactor
Nov 6, 2025
Merged

tests.overriding: refactor and allow non-bool expected value#456848
MattSturgeon merged 2 commits intoNixOS:masterfrom
ShamrockLee:test-overriding-refactor

Conversation

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Oct 29, 2025

  • Refactor test package generation:
    • Use Nix languages equality operator to compare expr and expected at evaluation time, instead of comparing their string-interpolation at build time.
    • Use structured attributes to pass testResults instead of dumping all the values into buildCommand.
    • Aside from passthru.tests, add passthru.failures for lib.runTest-generated list of failed tests, similar to lib/tests/misc.
  • Refactor test cases
    • Change all tests in the form
      {
        expr = realExpr == realExpected;
        expected = true;
      }
      to
      {
        expr = realExpr;
        expected = realExpected;
      }

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-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Oct 29, 2025
@ShamrockLee ShamrockLee force-pushed the test-overriding-refactor branch from da226c5 to 031e352 Compare October 29, 2025 23:53
@ShamrockLee ShamrockLee marked this pull request as draft October 29, 2025 23:53
@ShamrockLee ShamrockLee force-pushed the test-overriding-refactor branch from 031e352 to 1e22aeb Compare October 30, 2025 10:31
@nixpkgs-ci nixpkgs-ci bot removed the 9.needs: reviewer This PR currently has no reviewers requested and needs attention. label Oct 30, 2025
@ShamrockLee ShamrockLee marked this pull request as ready for review October 30, 2025 10:42
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Much better! I had the same thought of "why isn't expr compared with expected like in other tests" when I looked at this test.

Some thoughts/ideas/suggestions below to discuss, but nothing blocking:

passthru = { inherit tests; };
passthru = {
inherit tests;
failures = lib.runTests (finalAttrs.passthru.tests // { tests = lib.attrNames tests; });
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand a) why we need two test frameworks here (this derivation and runTests) and b) why this is called failures.

If we include this passthru, isn't it just an alternative to the below, that also attempts to print the expr and expected results using toPretty?

Copy link
Contributor Author

@ShamrockLee ShamrockLee Oct 30, 2025

Choose a reason for hiding this comment

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

isn't it just an alternative to the below, that also attempts to print the expr and expected results using toPretty?

IIUC, the design of tests.overriding avoids context from test cases, which would break if we print expr and expected in buildCommand.

tests.overriding is essentially a collection of evaluation tests for build helpers. But we need it as a package test, and therefore the package part. The name failures comes from a search across Nixpkgs with keyword "lib.runTests", and 3 in 5 result are failures = lib.runTests {.

I added it as an alternative to building the package (nix eval -f . tests.overriding.failures), which hopefully ease staging change testing. Still, having two test frameworks is kinda redundant, and we could just tell people to look at tests.overriding.tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's precedent for failures = runTests elsewhere, then I'm fine with having it. It just felt (feels) like a bit of an odd pattern.

@ShamrockLee ShamrockLee force-pushed the test-overriding-refactor branch from c4d97a4 to fea9839 Compare October 30, 2025 18:27
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM

@MattSturgeon MattSturgeon force-pushed the test-overriding-refactor branch from 57ad481 to ffca1c4 Compare November 6, 2025 00:18
Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

I squashed 57ad481a13faac26777246b9bd02c1e0dd48ab20. Hopefully the rebase also helps with the weird CI error we were seeing.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 6, 2025
@MattSturgeon
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 456848
Commit: ffca1c4197f52aa58e1c0e600b874bd0e00999df (subsequent changes)
Merge: 506ec31c6bddfa78305a8f6665bd8adb851a14ca

Logs: https://github.com/MattSturgeon/nixpkgs-review-gha/actions/runs/19120892935


x86_64-linux

✅ 1 package built:
  • tests.overriding

aarch64-linux

✅ 1 package built:
  • tests.overriding

x86_64-darwin (sandbox = true)

✅ 1 package built:
  • tests.overriding

aarch64-darwin (sandbox = true)

✅ 1 package built:
  • tests.overriding

MattSturgeon

This comment was marked as duplicate.

@MattSturgeon MattSturgeon added this pull request to the merge queue Nov 6, 2025
Merged via the queue into NixOS:master with commit fbafefc Nov 6, 2025
29 of 33 checks passed
@ShamrockLee ShamrockLee added the backport release-25.11 Backport PR automatically label Dec 12, 2025
@nixpkgs-ci

This comment has been minimized.

@ShamrockLee
Copy link
Contributor Author

Ah! It is already in release-25.11.

@ShamrockLee ShamrockLee removed the backport release-25.11 Backport PR automatically label Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants