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

Add functional tests for include directives in nix config file #10386

Merged

Conversation

SkamDart
Copy link
Contributor

@SkamDart SkamDart commented Apr 3, 2024

Motivation

Add two tests for the !include and include directives used in nix.conf

https://github.com/NixOS/nix/pull/10358/files

Context

Resolves #10383

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@SkamDart SkamDart requested a review from edolstra as a code owner April 3, 2024 01:22
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 3, 2024
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@thufschmitt thufschmitt merged commit 5536788 into NixOS:master Apr 3, 2024
8 checks passed
@@ -56,4 +66,4 @@ exp_features=$(nix config show | grep '^experimental-features' | cut -d '=' -f 2
# Test that it's possible to retrieve a single setting's value
val=$(nix config show | grep '^warn-dirty' | cut -d '=' -f 2 | xargs)
val2=$(nix config show warn-dirty)
[[ $val == $val2 ]]
[[ $val == $val2 ]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we conventionally have trailing newlines

Comment on lines +52 to +53
export NIX_USER_CONF_FILES=$here/config/nix-with-bang-include.conf
var=$(nix config show | grep '^experimental-features =' | cut -d '=' -f 2 | xargs)
Copy link
Member

@Ericson2314 Ericson2314 Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var=$(NIX_USER_CONF_FILES=$here/config/nix-with-bang-include.conf nix config show | grep '^experimental-features =' | cut -d '=' -f 2 | xargs)

I would do it this way so it doesn't interfere with the other tests

Comment on lines +47 to +48
export NIX_USER_CONF_FILES=$here/config/nix-with-include.conf
var=$(nix config show | grep '^allowed-uris =' | cut -d '=' -f 2 | xargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,2 @@
experimental-features = nix-command
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do a test like

system = asdf

because experimental-features is much more likely to interfere with the test suite itself

@Ericson2314
Copy link
Member

@SkamDart Thank you for quickly taking on this issue! It's certainly better than it was before, but these are a few small things that I think would make it better still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Test include and !include config file syntax
3 participants