Move unit tests to separate directories, and document #8886

Ericson2314 · 2023-08-30T23:11:32Z

Motivation

Today, with the tests inside a tests intermingled with the corresponding library's source code, we have a few problems:

We have to be careful that wildcards don't end up with tests being built as part of Nix proper, or test headers being installed as part of Nix proper.
Tests in libraries but not executables is not right:
- It means each executable runs the previous unit tests again, because
  it needs the libraries.
- It doesn't work right on Windows, which doesn't want you to load a DLL just for the side global variable . It could be made to work with the dlopen equivalent, but that's gross!

This reorg solves these problems.

There is a remaining problem which is that sibbling headers (like hash.hh the test header vs hash.hh the main libnixutil header) end up shadowing each other. This PR doesn't solve that. That is left as future work for a future PR.

Context

This does something more minimal than what #7876 asks for, but it is a step in the right direction.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Ericson2314 · 2023-08-31T22:03:33Z

I should add that the way things are today was quite concerning to @p01arst0rm when working on the Meson fork.

p01arst0rm · 2023-08-31T22:11:50Z

good PR 👍

doc/manual/src/contributing/testing.md

fricklerhandwerk · 2023-11-13T14:11:06Z

Discussed in Nix team meeting:

moving the nested tests -> test produces somewhat unnecessary diff
@thufschmitt: is there any cost to nesting all of that under a top-level tests directory?
- @Ericson2314: can be done
@edolstra: It isn't so nice that foo.cc (of tests) is no longer next to tests/foo.cc
- @Ericson2314: Can move inside tests dir
- But we do need the inner test dir
- @edolstra why is that?
- @Ericson2314: Lack of -I tests=... like Nix has in the C/C++ compiler
assigned to @thufschmitt

doc/internal-api/doxygen.cfg.in

nixos-discourse · 2023-11-13T18:50:21Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-11-13-nix-team-meeting-minutes-103/35400/1

Ericson2314 · 2023-11-20T23:09:04Z

Somewhat surprisingly, this has become relevant for the Windows work (I need to separate the test exes and tests libraries better, tests/foo.cc for lib vs foo.cc will do the trick).

Let me know if this looks good to you, @thufschmitt. (The conflicts are usually extremely superficial and easy to fix.)

thufschmitt

A couple of small things in the testing manual, but looks good otherwise.

Feel free to merge once it's addressed (or discarded, I don't feel too strongly about these)

doc/manual/src/contributing/testing.md

thufschmitt · 2023-11-21T05:42:11Z

doc/manual/src/contributing/testing.md

+### Rationale
+
+The use of a separate directory for the unit tests might seem inconvenient, as the tests are not "right next to" the part of the code they are testing.
+But organizing the tests this way has one big benefit:
+there is no risk of any build-system wildcards for the library accidentally picking up test code that shoud not built and installed as part of the library.
+
+Likewise, the use of the `test/` subdir might seem superfluous:
+Isn't the point of the `*-test` subdir to indicate that these files are tests?
+Why do we need another `test` subdirectory?
+The answer is that we need to be able to tell apart the two headers, and make sure we include the right one.
+For `.cc` files, this is matter of doing `#include "foo.hh"` vs `#include "tests/foo.hh`
+For `.hh` files, this is a bit more subtle, because a `#include "foo.hh` instead `test/foo.hh` will end up including itself because `#include "..."`
+[always prioritizes files in the same directory](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/c99.html#tag_20_11_04), rather than the original header from the library.
+Instead, use `#include <foo.hh>` to get the original library header, and `#include "tests/foo.hh"` to get the test header.
+
+Why do we have test headers at all?
+These are usually for [rapidcheck]'s `Arbitrary` machinery, which is used to describe how to generate values of a given type for the sake of running property tests.
+Because types contain other types, `Arbitrary` "instances" for some type are not just useful for testing that type, but also any other type that contains it.
+Indeed, if we don't reuse the upstream type's `Arbitrary` instance, the downstream type's `Arbitrary` instance would become much more complex and hard to understand.
+In order to reuse these instances, we therefore declare them in these testing headers.


Is don't think we care a bout that at all here. This is nice for a commit message, but users/contributors generally shouldn't be really concerned about it, and wouldn't read it any way.

I think it's the generally he right place for this kind of contextual information, but we may consider moving that out of the manual and into the tree. (At some point in the future, that is.)

I want contributors to also write property tests and "Arbitrary instances", so I do want that pattern to be documented.

Amid many other changes this text is now reorganized out of the file layout section. Hope that is better.

Ericson2314 · 2023-11-24T05:07:35Z

@thufschmitt I am embarrassed to say that my "I'll just fix conflicts after" was wrong, so I think this needs to be re-reviewed.

Here is what happened:

I realized putting the source files also in tests defeated one of the purposes purpose of the original PR, which is to make sure that #include "foo.h" without test/ unambiguously refereed to the original library header, and not the test header. Nevertheless, I kept the companions to test headers in test --- i.e. the PR does the thing we agreed on the meeting despite these issues and instead no longer attempts to address this ambiguous #include problem.
I better separate the "test libs" with headers and arbitrary instances from the tests themselves without headers:
- libraries get their own test/unit/*-lib dir.
- Now most tests .cc files do not need to go in tests/ because they don't correspond to a header
- This fixes running the unit tests on windows, where it doesn't want to load a separate DLL just for the side effects.
unit test data dirs are not longer a separate hierarchy, as avoiding "cumulative" test exes means we don't need a single root data dir.

So one problem is no longer solved in the name of concensus, but another problem is solved (the windows problem), so this PR is hopefully still just as net useful.

doc/manual/src/contributing/testing.md

nixos-discourse · 2023-11-27T16:08:49Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-11-27-nix-team-meeting-minutes-107/36112/1

roberth

Thoughts:

This moves from functional grouping to technical group, which is a small regression in DX, but that's something because meson requires it anyway, and meson will fix more important issues
Significant churn. @Ericson2314 commits to backporting and helping out with PR rebasing.
Fixes issues as described

Conclusion: net positive

Today, with the tests inside a `tests` intermingled with the corresponding library's source code, we have a few problems: - We have to be careful that wildcards don't end up with tests being built as part of Nix proper, or test headers being installed as part of Nix proper. - Tests in libraries but not executables is not right: - It means each executable runs the previous unit tests again, because it needs the libraries. - It doesn't work right on Windows, which doesn't want you to load a DLL just for the side global variable . It could be made to work with the dlopen equivalent, but that's gross! This reorg solves these problems. There is a remaining problem which is that sibbling headers (like `hash.hh` the test header vs `hash.hh` the main `libnixutil` header) end up shadowing each other. This PR doesn't solve that. That is left as future work for a future PR. Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

github-actions · 2023-12-01T16:16:13Z

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-8886-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-8886-to-2.18-maintenance
git switch --create backport-8886-to-2.18-maintenance
git cherry-pick -x 91b6833686a6a6d9eac7f3f66393ec89ef1d3b57

github-actions · 2023-12-01T16:16:14Z

Backport failed for 2.19-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-8886-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-8886-to-2.19-maintenance
git switch --create backport-8886-to-2.19-maintenance
git cherry-pick -x 91b6833686a6a6d9eac7f3f66393ec89ef1d3b57

github-actions · 2023-12-01T16:16:20Z

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-8886-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-8886-to-2.18-maintenance
git switch --create backport-8886-to-2.18-maintenance
git cherry-pick -x 91b6833686a6a6d9eac7f3f66393ec89ef1d3b57

github-actions · 2023-12-01T16:16:21Z

Backport failed for 2.19-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-8886-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-8886-to-2.19-maintenance
git switch --create backport-8886-to-2.19-maintenance
git cherry-pick -x 91b6833686a6a6d9eac7f3f66393ec89ef1d3b57

Ericson2314 requested review from thufschmitt, fricklerhandwerk and edolstra as code owners August 30, 2023 23:11

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Aug 30, 2023

cole-h reviewed Aug 31, 2023

View reviewed changes

doc/manual/src/contributing/testing.md Outdated Show resolved Hide resolved

Ericson2314 force-pushed the flatten-tests branch 2 times, most recently from f5d40b0 to 83d0e79 Compare September 2, 2023 18:31

fricklerhandwerk reviewed Sep 3, 2023

View reviewed changes

Ericson2314 force-pushed the flatten-tests branch from aa1c15a to 4852a5d Compare September 3, 2023 21:07

Ericson2314 mentioned this pull request Sep 5, 2023

Unit test some worker protocol serializers #8923

Merged

8 tasks

Ericson2314 force-pushed the flatten-tests branch from 4852a5d to 5b4c844 Compare September 26, 2023 15:39

Ericson2314 changed the title ~~Move tests to separate directories, and document~~ Move unit tests to separate directories, and document Oct 6, 2023

Ericson2314 force-pushed the flatten-tests branch 3 times, most recently from ad98a09 to 5705c8d Compare October 9, 2023 21:55

fricklerhandwerk approved these changes Oct 9, 2023

View reviewed changes

Ericson2314 force-pushed the flatten-tests branch from 5705c8d to 8d554a9 Compare October 23, 2023 17:50

thufschmitt self-assigned this Nov 13, 2023

fricklerhandwerk reviewed Nov 13, 2023

View reviewed changes

doc/internal-api/doxygen.cfg.in Outdated Show resolved Hide resolved

Ericson2314 force-pushed the flatten-tests branch from 8d554a9 to d096395 Compare November 13, 2023 23:08

thufschmitt approved these changes Nov 21, 2023

View reviewed changes

Ericson2314 force-pushed the flatten-tests branch 2 times, most recently from c10b7c7 to ee03d39 Compare November 24, 2023 04:53

fricklerhandwerk reviewed Nov 27, 2023

View reviewed changes

doc/manual/src/contributing/testing.md Outdated Show resolved Hide resolved

Ericson2314 mentioned this pull request Nov 30, 2023

Build a minimized Nix with MinGW #8901

Merged

8 tasks

Ericson2314 force-pushed the flatten-tests branch 3 times, most recently from 10f8db7 to 0f54ece Compare December 1, 2023 14:33

roberth approved these changes Dec 1, 2023

View reviewed changes

roberth enabled auto-merge December 1, 2023 14:56

Ericson2314 force-pushed the flatten-tests branch from 0f54ece to 91b6833 Compare December 1, 2023 15:53

This was referenced Dec 1, 2023

[Backport 2.19-maintainence] nixpkgs bumps #9513

Merged

[Backport 2.18-maintainence] nixpkgs bump #9514

Merged

roberth merged commit 84fe429 into NixOS:master Dec 1, 2023
8 checks passed

Ericson2314 deleted the flatten-tests branch December 1, 2023 16:15

Ericson2314 added backport 2.18-maintenance Automatically creates a PR against the branch backport 2.19-maintenance Automatically creates a PR against the branch labels Dec 1, 2023

This was referenced Dec 1, 2023

[Backport 2.19-maintanence] Move tests to separate directories, and document #9516

Merged

[Backport 2.18-maintanence] Backport test source layout reorgs #9517

Merged

ShamrockLee mentioned this pull request Feb 26, 2024

.gitignore: ignore historical test binaries #10085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move unit tests to separate directories, and document #8886

Move unit tests to separate directories, and document #8886

Ericson2314 commented Aug 30, 2023 •

edited

Loading

Ericson2314 commented Aug 31, 2023

p01arst0rm commented Aug 31, 2023

fricklerhandwerk commented Nov 13, 2023 •

edited

Loading

nixos-discourse commented Nov 13, 2023

Ericson2314 commented Nov 20, 2023

thufschmitt left a comment

thufschmitt Nov 21, 2023

fricklerhandwerk Nov 21, 2023 •

edited

Loading

Ericson2314 Nov 21, 2023

Ericson2314 Nov 24, 2023

Ericson2314 commented Nov 24, 2023

nixos-discourse commented Nov 27, 2023

roberth left a comment

github-actions bot commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

Move unit tests to separate directories, and document #8886

Move unit tests to separate directories, and document #8886

Conversation

Ericson2314 commented Aug 30, 2023 • edited Loading

Motivation

Context

Checklist for maintainers

Priorities

Ericson2314 commented Aug 31, 2023

p01arst0rm commented Aug 31, 2023

fricklerhandwerk commented Nov 13, 2023 • edited Loading

nixos-discourse commented Nov 13, 2023

Ericson2314 commented Nov 20, 2023

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Nov 21, 2023

Choose a reason for hiding this comment

fricklerhandwerk Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Ericson2314 Nov 21, 2023

Choose a reason for hiding this comment

Ericson2314 Nov 24, 2023

Choose a reason for hiding this comment

Ericson2314 commented Nov 24, 2023

nixos-discourse commented Nov 27, 2023

roberth left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

Ericson2314 commented Aug 30, 2023 •

edited

Loading

fricklerhandwerk commented Nov 13, 2023 •

edited

Loading

fricklerhandwerk Nov 21, 2023 •

edited

Loading