Skip to content

Tests for derivation "advanced attrs"#10874

Merged
Ericson2314 merged 3 commits intoNixOS:masterfrom
haenoe:derivation-tests
Jun 24, 2024
Merged

Tests for derivation "advanced attrs"#10874
Ericson2314 merged 3 commits intoNixOS:masterfrom
haenoe:derivation-tests

Conversation

@haenoe
Copy link
Contributor

@haenoe haenoe commented Jun 9, 2024

Motivation

Closely linked to #10760, which aims to change the internal data-structures surrounding Derivation.
The goal of this PR is to ensure that the other PR does not change any current behavior.

This is very much work-in-progress, so happy about every piece of feedback!

Context

See #10760

@Ericson2314 Broke up the builtins.derivationStrict implementation in order to expose creating a (C++) Derivation for the sake of these unit tests.

Priorities and Process

Add 👍 to pull requests you find important.

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

@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Jun 9, 2024
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Can't comment on everything, but here's something.
Anyway, it's great that you're adding tests.

@haenoe haenoe marked this pull request as ready for review June 13, 2024 12:14
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

This looks quite good to me now!

@Ericson2314 Ericson2314 changed the title Derivation tests Unit test for derivation "advanced attrs" Jun 14, 2024
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Reviewed live. Only question is about the future of "dummy". Tests help create more confidence in upcoming refactors.

@Ericson2314
Copy link
Member

This no longer depends on changing dummy://

Setting the global settings in unit tests like this is very ugly, but the real problem is not changing the settings but having the code being unit tested depend on global variables at all. #5638 tracks fixing this.

I intend not to let that issue fester; #10913 is some progress on it.

@fricklerhandwerk fricklerhandwerk added with-tests Issues related to testing. PRs with tests have some priority tests labels Jun 17, 2024
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-06-17-nix-team-meeting-minutes-153/47186/1

@haenoe haenoe force-pushed the derivation-tests branch from ded4824 to 84b53a1 Compare June 21, 2024 13:29
@haenoe haenoe changed the title Unit test for derivation "advanced attrs" Tests for derivation "advanced attrs" Jun 21, 2024
Will be used in a second test after `lang.sh`.
@Ericson2314 Ericson2314 dismissed edolstra’s stale review June 23, 2024 19:48

It now uses a functional test!

@Ericson2314 Ericson2314 force-pushed the derivation-tests branch 2 times, most recently from 96de33a to 8b109b5 Compare June 24, 2024 00:41
@Ericson2314 Ericson2314 enabled auto-merge June 24, 2024 00:41
haenoe and others added 2 commits June 23, 2024 21:42
This tests the Nix language side of things.

We are purposely skipping most of `common.sh` because it is overkill for
this test: we don't want to have an "overfit" test environment.

Co-Authored-By: John Ericson <John.Ericson@Obsidian.Systems>
This tests the parser and JSON format using the DRV files from the tests
added in the previous commit.

Co-Authored-By: John Ericson <John.Ericson@Obsidian.Systems>
@Ericson2314 Ericson2314 merged commit 927b719 into NixOS:master Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

store Issues and pull requests concerning the Nix store tests with-tests Issues related to testing. PRs with tests have some priority

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants