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.

Sign up for GitHub

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

Jump to bottom

Test nix build --json return output paths in floating CA case #4589

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Test nix build --json return output paths in floating CA case #4589

merged 1 commit into from
Feb 13, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 1, 2021 •
edited by tomberek
Loading

Motivation

Adding a test to ensure there is no regression.

The tests that are split out of tests/build.sh are ones that don't yet work with CA derivation. I have not yet evaluated whether they should or not.

Context

This behavior, reported missing in issue #4661, already got fixed in PR #4818, but didn't get a test case then.

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
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

thufschmitt
thufschmitt reviewed Mar 1, 2021
View reviewed changes
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.

To make the types more explicit (and also prevent a couple of unneeded back-and-forths), could it make sense to make build return a vector of RealisedPath?

tests/build-floating-ca-output.sh Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Aug 28, 2021

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

@stale stale bot added the stale label Aug 28, 2021
@stale stale bot removed the stale label Oct 1, 2021
@Ericson2314 Ericson2314 changed the title Make nix build --json return output paths in floating CA case Test nix build --json return output paths in floating CA case Oct 1, 2021
@Ericson2314
Copy link
Member Author

This is now just a test, and ready to go.

@Ericson2314 Ericson2314 mentioned this pull request Oct 1, 2021
Merged
@edolstra
Copy link
Member

edolstra commented Oct 4, 2021

Regarding new-cli, there are already a lot of tests of the nix command in the top-level tests directory, so this could get confusing.

@Ericson2314
Copy link
Member Author

@edolstra would you prefer I flatten this, or put those in the subdir?

@edolstra
Copy link
Member

edolstra commented Oct 5, 2021

@Ericson2314 Yes please. nix build tests can go in tests/build.sh (which already has a nix build test), and CA tests can go in tests/ca.

@Ericson2314
Copy link
Member Author

Oh i meant to replace tests/build.sh --- I was trying to separating test the build command vs building itself.

But I'll do it the way you said for now.

Ericson2314
Ericson2314 commented Oct 7, 2021
View reviewed changes
tests/build.sh Outdated Show resolved Hide resolved
edolstra
edolstra reviewed Oct 7, 2021
View reviewed changes
tests/multiple-outputs.nix Outdated Show resolved Hide resolved
@Ericson2314
Copy link
Member Author

Found a bunch of testing infra that indeed already exists since I first wrote. This became a lot easier now!

@stale
Copy link

stale bot commented Apr 16, 2022

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

@stale stale bot added the stale label Apr 16, 2022
@stale stale bot removed the stale label Feb 2, 2023
Ericson2314
Ericson2314 commented Feb 2, 2023
View reviewed changes
tests/build-delete.sh

set -o pipefail

# https://github.com/NixOS/nix/issues/6572
Copy link
Member Author

@Ericson2314 Ericson2314 Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These tests do not yet work with CA derivations, so I pulled them out from tests/build.sh into their own file.

Ericson2314
Ericson2314 commented Feb 2, 2023
View reviewed changes
tests/output-normalization.sh
@@ -0,0 +1,9 @@
source common.sh

testNormalization () {
Copy link
Member Author

@Ericson2314 Ericson2314 Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This test also do not yet work with CA derivations, so I pulled it out from tests/build.sh into its own file.

@Ericson2314
Test nix build --json return output paths in floating CA case
55016b6 
Adding a test to ensure there is no regression.

The tests that are split out of `tests/build.sh` are ones that don't yet
work with CA derivation. I have not yet evaluated whether they should or
not.

This behavior, reported missing in issue NixOS#4661, already got fixed in
PR NixOS#4818, but didn't get a test case then.
Ericson2314
Ericson2314 commented Feb 12, 2023
View reviewed changes
tests/ca/new-build-cmd.sh

export NIX_TESTS_CA_BY_DEFAULT=1
cd ..
source ./build.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

This is us running the (remaining) tests/build.sh tests, except with CA derivations enabled by default. This shows that this functionality works with or with or without CA derivations.

@tomberek tomberek merged commit df9a71f into NixOS:master Feb 13, 2023
@Ericson2314 Ericson2314 deleted the better-build-ca-json branch February 13, 2023 14:08
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-02-10-nix-team-meeting-minutes-31/25438/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@thufschmitt thufschmitt thufschmitt left review comments

@edolstra edolstra edolstra left review comments

@tomberek tomberek tomberek approved these changes

Assignees

@tomberek tomberek

Labels
None yet
Projects
Nix team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Ericson2314 @edolstra @nixos-discourse @tomberek @thufschmitt @SuperSandro2000