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

Put functional tests in tests/functional #9103

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

Ericson2314
Copy link
Member

Motivation

I think it is bad for these reasons when tests/ contains a mix of functional and integration tests

  • Concepts is harder to understand, the documentation makes a good unit vs functional vs integration distinction, but when the integration tests are just two subdirs within tests/ this is not clear.

  • Source filtering in the flake.nix is more complex. We need to filter out some of the dirs from tests/, rather than simply pick the dirs we want and take all of them. This is a good sign the structure of what we are trying to do is not matching the structure of the files.

With this change we have a clean:

$ git show 'HEAD:tests'
tree HEAD:tests

functional/
installer/
nixos/

Context

I suppose this is part of #7876

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Oct 5, 2023
@Ericson2314
Copy link
Member Author

Ericson2314 commented Oct 5, 2023

If anyone thinks tests/functional is too long, I am open to other things, include 3 top level dirs (e.g. func-tests nixos-tests installer-tests),

@roberth roberth added the contributor-experience Developer experience for Nix contributors label Oct 5, 2023
I think it is bad for these reasons when `tests/` contains a mix of
functional and integration tests

 - Concepts is harder to understand, the documentation makes a good
   unit vs functional vs integration distinction, but when the
   integration tests are just two subdirs within `tests/` this is not
   clear.

 - Source filtering in the `flake.nix` is more complex. We need to
   filter out some of the dirs from `tests/`, rather than simply pick
   the dirs we want and take all of them. This is a good sign the
   structure of what we are trying to do is not matching the structure
   of the files.

With this change we have a clean:
```shell-session
$ git show 'HEAD:tests'
tree HEAD:tests

functional/
installer/
nixos/
```
@Ericson2314 Ericson2314 force-pushed the functional-tests-dir branch from bb9554f to 68c81c7 Compare October 6, 2023 13:06
@Ericson2314
Copy link
Member Author

@edolstra was wondering about whether this would induce conflicts. I am happy to report that I made this commit atop this branch, and it cherry-picked it all the way back to 2.3-maintainence with no conflict!

commit 6dfde324fb9dc898a7130932089a3c3553af053e (functional-tests-dir)
Author: John Ericson <[email protected]>
Date:   Fri Oct 6 09:06:38 2023 -0400

    asdf

diff --git a/tests/functional/fixed.nix b/tests/functional/fixed.nix
index babe71504..fd5a097f7 100644
--- a/tests/functional/fixed.nix
+++ b/tests/functional/fixed.nix
@@ -34,7 +34,7 @@ rec {
     # But Nix sees that an output with the desired hash already
     # exists, and will refrain from building it.
     (f ./fixed.builder2.sh "flat" "md5" "8ddd8be4b179a529afa5f2ffae4b9858")
-  ];
+  ]; # adsfasdf

   sameAsAdd =
     f ./fixed.builder2.sh "recursive" "sha256" "1ixr6yd3297ciyp9im522dfxpqbkhcw0pylkb2aab915278fqaik";

@Ericson2314 Ericson2314 enabled auto-merge October 6, 2023 13:12
@Ericson2314 Ericson2314 merged commit 61720d0 into NixOS:master Oct 6, 2023
8 checks passed
@Ericson2314 Ericson2314 deleted the functional-tests-dir branch October 17, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants