-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run test suite on NixOS #7778
Run test suite on NixOS #7778
Conversation
This also makes the path ctrl+clickable in vscode.
src = lib.cleanSourceWith { | ||
src = ./.; | ||
filter = path: type: | ||
toString path != toString ./tests/nixos | ||
&& toString path != toString ./flake.nix | ||
; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will be removed.
An improved change can be made after
Ideally the filter is additive - not subtractive - as subtractive filter don't scale well to projects with multiple derivation, which is increasingly the case for NixOS/nix
.
if (evalSettings.pureEval) { | ||
// TODO feature flag? | ||
// state.debugThrowLastTrace(EvalError({ | ||
// .msg = hintfmt("'%s' is not allowed in pure evaluation mode", "builtins.storePath"), | ||
// .errPos = state.positions[pos] | ||
// }));' | ||
state.allowPath(uncheckedPath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look for a workaround, as I know this is controversial. builtins.storePath
is referentially transparent, and only undesirable for some. If I can find an alternative for this change, I'll use that instead.
@@ -1,5 +1,8 @@ | |||
source common.sh | |||
|
|||
enableFeatures nix-command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had to add a lot of these enableFeatures
, as I could not find the supposed central place where they should be enabled. Doing it on a case by case basis made more sense to me, but if the in-build tests don't enforce these calls that's going to be annoying. The in-build tests should cover almost everything, so that the VM tests are almost guarnateed to succeed after that.
@@ -1,5 +1,8 @@ | |||
source common.sh | |||
|
|||
enableFeatures nix-command | |||
unknownFailsOnNixOS # fails because of path may have been built and not gc-ed; generate unique derivation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to resolve some of these later, so that we don't have to block this PR for too long.
isTestOnSystemNix() { | ||
[[ "${TEST_SYSTEM_NIX:-}" = 1 ]] | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions should be made a bit more accurate, and "integrate" better with the existing helper functions, which should also be made a bit more accurate or precise.
# Tests we don't need | ||
echo >tests/plugins/local.mk | ||
sed -i tests/local.mk \ | ||
-e 's^plugins\.sh^^' \ | ||
-e "s^test-deps += tests/plugins/libplugintest.\$(SO_EXT)^^" \ | ||
; | ||
|
||
export TEST_SYSTEM_NIX=1 | ||
export version=${config.nix.package.version} | ||
export NIX_REMOTE_=daemon | ||
export NIX_REMOTE=daemon | ||
make -j1 tests/eval-store.sh.test installcheck --keep-going |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These adaptation should be in an entrypoint that can be run outside of the VM tests. Benefits
- Quicker iteration on reproducers and improvements in test coverage that don't require main code changes
- Easier troubleshooting of test suite issues
nix.package = (builtins.getFlake "git+file:///home/user/h/nix?rev=a1c7cb8fb6c6511daf4884e140aba8ae1788cd84").packages.x86_64-linux.default; | ||
nix.settings.substituters = lib.mkForce []; | ||
|
||
nixpkgs.overlays = [ | ||
(final: prev: { | ||
thisNix = prev.nix; | ||
nix = config.nix.package; | ||
}) | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the nix.package
and overlay hacks here is to not rebuild the nix
derivation every time the test suite is changed during local development. This hack should be moved to a convenience attribute and not run on CI. It should use HEAD
instead of a specific hash.
Alternatively, we could have a mode that adds an impurely built (devshell) nix into the store somehow, or we could build Nix in such a way that it does not depend on its test suite.
- Possibly helpful is Track test derivations and parallelize building and testing #7662, to recover the testedness guarantee (or work around it by adding an indirection that runs both types of test)
Will re-do with fewer changes (and probably less coverage at first) |
|
Motivation
This one was a rabbit hole.
TODO
Context
Testing is how we make sure that we
don't break too many things
can trust contributions to do the right thing
can ask contributors to write good tests
Test everything before merging with merge queue + hercules-ci #7674
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*