Skip to content

nixos/test-driver: use attrname from nodes to refer to machines#181482

Closed
Sohalt wants to merge 2 commits intoNixOS:masterfrom
Sohalt:test-driver-machine-name
Closed

nixos/test-driver: use attrname from nodes to refer to machines#181482
Sohalt wants to merge 2 commits intoNixOS:masterfrom
Sohalt:test-driver-machine-name

Conversation

@Sohalt
Copy link
Contributor

@Sohalt Sohalt commented Jul 14, 2022

Description of changes

The NixOS test-driver sets up VMs as specified by the nodes attrset passed to makeTest. The VMs can then be interacted with from the python testScript using variables named after system.name of the VM. This breaks tests when system.name changes (e.g. because you like to add a git SHA to the name).

This change makes it possible to refer to machines by the attrname for the
config in the nodes attrset as opposed to the system.name, e.g.

makeTest = {
    nodes = {
        foo = { pkgs, ... }: {
            ...
        };
        bar = { pkgs, ... }: {
            ...
        };
    };
    testScript = ''
        foo.start()
        bar.start()
        ...
    '';
}

I think this way is strictly better, because the attrnames in nodes are guaranteed to be unique, they are usually defined right next to the testScript and they are completely independent from other parts of the module system (i.e. only the test-driver uses them).

The code is not very clean and not backwards compatible. I just want to have a starting point for discussion.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

This makes it possible to refer to machines by the attrname for the
config in the `nodes` attrset as opposed to the `system.name`, e.g.

makeTest = {
    nodes = {
        foo = { pkgs, ... }: {
            ...
        };
        bar = { pkgs, ... }: {
            ...
        };
    };
    testScript = ''
        foo.start()
        bar.start()
        ...
    '';
}
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jul 14, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jul 14, 2022
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.

I agree about the goal and it doesn't seem to be terribly backwards incompatible.

Note to self: this conflicts superficially with #176557; can easily be moved to the new files.

nodesList = (lib.attrNames nodes);
in nodesList ++ lib.optional (lib.length nodesList == 1 && !lib.elem "machine" nodesList) "machine";

# TODO: This is an implementation error and needs fixing
Copy link
Member

Choose a reason for hiding this comment

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

This comment was once added to acknowledge the problem that these names were too coupled.

When the nodes.<name> names correspond to python identifiers only, and to hostnames/system names only by their default value, we can get rid of this comment and invalidNodeNames.

We may still want to validate the attribute names to be valid identifiers, but as an alternative, we could use a dict of nodes in the test script. I don't have a much of an opinion about this choice.

vms = map (m: m.config.system.build.vm) (lib.attrValues nodes);

nodeHostNames = let
nodesList = map (c: c.config.system.name) (lib.attrValues nodes);
Copy link
Member

Choose a reason for hiding this comment

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

This line has moved to nixos/lib/testing/driver.nix.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@Sohalt Sohalt closed this Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

Development

Successfully merging this pull request may close these issues.

3 participants