-
-
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 unit tests with clean config #10562
Conversation
namespace nix { | ||
|
||
int preInitForTests(int argc, char ** argv) | ||
{ |
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.
Unchanged code from here down.
tests/unit/libstore/main.cc
Outdated
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 libstore test suite ironically did not exhibit the problem, because it was the C API tests that triggered the problem, which are mostly libexpr focused. (nix_string_realise
)
Unit tests that exercise build code should be possible to add though, so I've applied the same solution here.
3a9d696
to
9783425
Compare
Wouldn't it be easier to do what we do in |
I think it's cleaner to not even look in those places. Ideally we wouldn't even have such globals, and then we can trivially clean up the new code I wrote here. |
Doing it in C++ also helps with portability. |
There are global variables in libutil I am afraid too (the experimental feature settings). Would it be possible do this less imperatively in the C++, e.g. adding arguments to the C++ init functions? (OK if the C interface is still stateful since compat is more of an issue there.) |
9783425
to
503ceda
Compare
Good call. A new flag is a much simpler solution, and we don't need a particularly extension pattern at this time (or ever, hopefully). |
Part of why the diff is smaller now is that I'll do separately the part where libstore tests become capable of building. |
src/libmain/shared.hh
Outdated
@@ -9,6 +9,7 @@ | |||
#include "path.hh" | |||
#include "derived-path.hh" | |||
#include "exit.hh" | |||
#include "globals.hh" |
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.
Still needed?
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.
It wasn't!
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.
Except for my little note
This reverts commit 62feb5c. It runs as part of the functional tests, which control the environment, solving some of the problems a default config has when run in the sandbox.
110529f
to
1b6cd1d
Compare
Fixed in NixOS#10562 Building in unit tests should perhaps be kept to a minimum, but I have to note: * single derivations are pretty fast to build * we need to be able to build in various environment for the functional tests anyway * this was the only way to test that part of the C API * mocking only works if you have exceptionally good interfaces and the mocking code is reviewed over and over, if it works at all For what it's worth, I think we can have an "exceptionally good interface", in the form of NixOS#10579 Until that's been worked out, these cheap builds are fine to run in unit tests.
Motivation
@edolstra found an error running the tests in the dev shell.
It turns out the normal initialization still ran, and loaded configuration that caused the error.
This PR disables the loading of external configuration during unit test execution, and makes it possible for test suites for C API based bindings to do the same.
Context
The error was:
This error was only recently possible, as #10412 was the first PR to run small builds as part of the unit tests.
Building in unit tests should perhaps be kept to a minimum, but I have to note:
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.