Skip to content
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

[backport 2.3] Allow testing with different daemons #5650

Draft
wants to merge 14 commits into
base: 2.3-maintenance
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Nov 25, 2021

The goal is to make something like testNixVersions on master which can run the install checks against two pre-built nix versions. The goal is to then use that function in a later version of Nix (to avoid cycle dependencies!) to check 2.3's (modernized) test suite against a 2.3 client but newer daemon.

Concretely what his PR is doing is cherry-picking a number of test suite improves to 2.3 from later versions that generally harden the testsuite and in particular allow for this cross-version testing.

  • Cherry-pick commits
  • Revise commits to ensure test suite still runs the usual way
  • Turn a "at least 2.4pre..." to "no later than 2.4pre..."

For later:

  • Hand-backport the new bits in flake.nix to this release.nix.
  • Test with newer daemon marking fixing bugs and/or marking tests broken

depends on #5649
depends on #5753
depends on #5754
Depends on #9495

@thufschmitt
Copy link
Member

Couldn’t we rather use master’s testsuite with an old client (adding guards like the existing requireNewerDaemonThan)? That would remove the need for backporting anything to the testuite

@Ericson2314
Copy link
Member Author

@regnat the command line has changed so much, I expect that to be a big plain. Think of all those --impure --expr everywhere.

Also remember this is a 1-time cost. Going forward we already have the support we we need to e.g. test 2.4 with master daemon.

@dpulls
Copy link

dpulls bot commented Dec 9, 2021

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 force-pushed the 2.3-alt-daemon-test-suite branch 2 times, most recently from 6d6502c to c4c448c Compare December 9, 2021 15:44
@Ericson2314 Ericson2314 marked this pull request as draft December 9, 2021 19:40
@stale
Copy link

stale bot commented Jun 12, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Jun 12, 2022
@stale stale bot removed the stale label Feb 23, 2023
@dpulls
Copy link

dpulls bot commented Mar 8, 2023

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 marked this pull request as ready for review October 31, 2023 15:12
thufschmitt and others added 13 commits October 31, 2023 12:16
The `remote-store` test loads the `user-env` one to test nix-env when
using the daemon, but actually does it incorrectly because every test
starts (in `common.sh`) by resetting the value of `NIX_REMOTE`, meaning
that the `user-env` test will never use the daemon.

Fix this by setting `NIX_REMOTE_` before sourcing `user-env.sh` in the
`remote-store` test, so that `NIX_REMOTE` is correctly set inside the
test

(cherry picked from commit f6ac888)
This requires adding `nix` to its own closure which is a bit unfortunate,
but as it is optional (the test will be disabled if `OUTER_NIX` is unset) it
shouldn't be too much of an issue.

(Ideally this should go in another derivation so that we can build Nix and run
the test independently, but as the tests are running in the same derivation
as the build it's a bit complicated to do so).

(cherry picked from commit 5716345)
That way we can run them without rebuilding Nix

(cherry picked from commit a0866c8)
When `NIX_DAEMON_PACKAGE` is set, make all the tests use the Nix daemon.
That way we can test every piece of Nix functionality both with and
without the daemon.

Tests for which using the daemon isn’t possible or doesn’t make sens can
selectively be disabled with `needLocalStore`

(cherry picked from commit addacfc)
For some reason, an old socket occasionally stays here on OSX, causing
the subsequent tests to fail

(cherry picked from commit c2c0dba)
- Don’t hardcode the “newer” version
- Remove an ill-placed `return`

(cherry picked from commit 3a2fc9c)
Having the `post-build-hook` use `nix` from the client package can lead
to a deadlock in case there’s a db migration to do between both, as a
`nix` command running inside the hook will run as root (and as such will
bypass the daemon), so might trigger a db migration, which will get
stuck trying to get a global lock on the DB (as the daemon that ran the
hook already has a lock on it).

(cherry picked from commit 93eadd5)
The eventual PATH entry needs the `.../bin` or we will not use the right
daemon.

(cherry picked from commit 06fb6ae)
We want the old behavior, since this is Nix 2.3.
Add an `$` at the end of the `grep` regex. Without it, `checkRef foo`
would always imply `checkRef foo.drv`. We want to tell these situations
apart to more precisely test what is going on.

(cherry picked from commit f587598)
(cherry picked from commit 5dbbf23)
Use `set -u` and `set -o pipefail` to catch accidental mistakes and
failures more strongly.

 - `set -u` catches the use of undefined variables
 - `set -o pipefail` catches failures (like `set -e`) earlier in the
   pipeline.

This makes the tests a bit more robust. It is nice to read code not
worrying about these spurious success paths (via uncaught) errors
undermining the tests. Indeed, I caught some bugs doing this.

There are a few tests where we run a command that should fail, and then
search its output to make sure the failure message is one that we
expect. Before, since the `grep` was the last command in the pipeline
the exit code of those failing programs was silently ignored. Now with
`set -o pipefail` it won't be, and we have to do something so the
expected failure doesn't accidentally fail the test.

To do that we use `expect` and a new `expectStderr` to check for the
exact failing exit code. See the comments on each for why.

`grep -q` is replaced with `grepQuiet`, see the comments on that
function for why.

`grep -v` when we just want the exit code is replaced with `grepInverse,
see the comments on that function for why.

`grep -q -v` together is, surprise surprise, replaced with
`grepQuietInverse`, which is both combined.

(cherry picked from commit c118361)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants