Merge pull request #228111 from alyssais/nixosTest-hostname

nixosTest: remove hostname limitations

nixos/doc/manual/development/developing-the-test-driver.chapter.md

@@ -25,6 +25,8 @@ These include `pkgs.nixosTest`, `testing-python.nix` and `make-test-python.nix`.
 
 ## Testing changes to the test framework {#sec-test-the-test-framework}
 
+We currently have limited unit tests for the framework itself. You may run these with `nix-build -A nixosTests.nixos-test-driver`.
+
 When making significant changes to the test framework, we run the tests on Hydra, to avoid disrupting the larger NixOS project.
 
 For this, we use the `python-test-refactoring` branch in the `NixOS/nixpkgs` repository, and its [corresponding Hydra jobset](https://hydra.nixos.org/jobset/nixos/python-test-refactoring).

nixos/doc/manual/development/writing-nixos-tests.section.md

@@ -130,6 +130,11 @@ starting them in parallel:
 start_all()
 ```
 
+If the hostname of a node contains characters that can't be used in a
+Python variable name, those characters will be replaced with
+underscores in the variable name, so `nodes.machine-a` will be exposed
+to Python as `machine_a`.
+
 ## Machine objects {#ssec-machine-objects}
 
 The following methods are available on machine objects:

nixos/lib/test-driver/test_driver/driver.py

@@ -2,6 +2,7 @@ from contextlib import contextmanager
 from pathlib import Path
 from typing import Any, Dict, Iterator, List, Union, Optional, Callable, ContextManager
 import os
+import re
 import tempfile
 
 from test_driver.logger import rootlog
@@ -28,6 +29,10 @@ def get_tmp_dir() -> Path:
     return tmp_dir
 
 
+def pythonize_name(name: str) -> str:
+    return re.sub(r"^[^A-z_]|[^A-z0-9_]", "_", name)
+
+
 class Driver:
     """A handle to the driver that sets up the environment
     and runs the tests"""
@@ -113,7 +118,7 @@ class Driver:
             polling_condition=self.polling_condition,
             Machine=Machine,  # for typing
         )
-        machine_symbols = {m.name: m for m in self.machines}
+        machine_symbols = {pythonize_name(m.name): m for m in self.machines}
         # If there's exactly one machine, make it available under the name
         # "machine", even if it's not called that.
         if len(self.machines) == 1:

nixos/lib/testing/driver.nix

@@ -21,29 +21,20 @@ let
     in
     nodesList ++ lib.optional (lib.length nodesList == 1 && !lib.elem "machine" nodesList) "machine";
 
-  # TODO: This is an implementation error and needs fixing
+  pythonizeName = name:
-  # the testing famework cannot legitimately restrict hostnames further
+    let
-  # beyond RFC1035
+      head = lib.substring 0 1 name;
-  invalidNodeNames = lib.filter
+      tail = lib.substring 1 (-1) name;
-    (node: builtins.match "^[A-z_]([A-z0-9_]+)?$" node == null)
+    in
-    nodeHostNames;
+      (if builtins.match "[A-z_]" head == null then "_" else head) +
+      lib.stringAsChars (c: if builtins.match "[A-z0-9_]" c == null then "_" else c) tail;
 
   uniqueVlans = lib.unique (builtins.concatLists vlans);
   vlanNames = map (i: "vlan${toString i}: VLan;") uniqueVlans;
-  machineNames = map (name: "${name}: Machine;") nodeHostNames;
+  pythonizedNames = map pythonizeName nodeHostNames;
+  machineNames = map (name: "${name}: Machine;") pythonizedNames;
 
-  withChecks =
+  withChecks = lib.warnIf config.skipLint "Linting is disabled";
-    if lib.length invalidNodeNames > 0 then
-      throw ''
-        Cannot create machines out of (${lib.concatStringsSep ", " invalidNodeNames})!
-        All machines are referenced as python variables in the testing framework which will break the
-        script when special characters are used.
-
-        This is an IMPLEMENTATION ERROR and needs to be fixed. Meanwhile,
-        please stick to alphanumeric chars and underscores as separation.
-      ''
-    else
-      lib.warnIf config.skipLint "Linting is disabled";
 
   driver =
     hostPkgs.runCommand "nixos-test-driver-${config.name}"
@@ -87,7 +78,7 @@ let
         ${testDriver}/bin/generate-driver-symbols
         ${lib.optionalString (!config.skipLint) ''
           PYFLAKES_BUILTINS="$(
-            echo -n ${lib.escapeShellArg (lib.concatStringsSep "," nodeHostNames)},
+            echo -n ${lib.escapeShellArg (lib.concatStringsSep "," pythonizedNames)},
             < ${lib.escapeShellArg "driver-symbols"}
           )" ${hostPkgs.python3Packages.pyflakes}/bin/pyflakes $out/test-script
         ''}

nixos/tests/all-tests.nix

@@ -66,6 +66,15 @@ let
     ;
 
 in {
+
+  # Testing the test driver
+  nixos-test-driver = {
+    extra-python-packages = handleTest ./nixos-test-driver/extra-python-packages.nix {};
+    node-name = runTest ./nixos-test-driver/node-name.nix;
+  };
+
+  # NixOS vm tests and non-vm unit tests
+
   _3proxy = runTest ./3proxy.nix;
   aaaaxy = runTest ./aaaaxy.nix;
   acme = runTest ./acme.nix;
@@ -220,7 +229,6 @@ in {
   etcd-cluster = handleTestOn ["x86_64-linux"] ./etcd-cluster.nix {};
   etebase-server = handleTest ./etebase-server.nix {};
   etesync-dav = handleTest ./etesync-dav.nix {};
-  extra-python-packages = handleTest ./extra-python-packages.nix {};
   evcc = handleTest ./evcc.nix {};
   fancontrol = handleTest ./fancontrol.nix {};
   fcitx5 = handleTest ./fcitx5 {};

nixos/tests/nixos-test-driver/extra-python-packages.nix

@@ -1,4 +1,4 @@
-import ./make-test-python.nix ({ ... }:
+import ../make-test-python.nix ({ ... }:
   {
     name = "extra-python-packages";
 

nixos/tests/nixos-test-driver/node-name.nix

@@ -0,0 +1,33 @@
+{
+  name = "nixos-test-driver.node-name";
+  nodes = {
+    "ok" = { };
+
+    # Valid node name, but not a great host name.
+    "one_two" = { };
+
+    # Valid node name, good host name
+    "a-b" = { };
+
+    # TODO: would be nice to test these eval failures
+    # Not allowed by lib/testing/network.nix (yet?)
+    # "foo.bar" = { };
+    # Not allowed.
+    # "not ok" = { }; # not ok
+  };
+
+  testScript = ''
+    start_all()
+
+    with subtest("python vars exist and machines are reachable through test backdoor"):
+      ok.succeed("true")
+      one_two.succeed("true")
+      a_b.succeed("true")
+
+    with subtest("hostname is derived from the node name"):
+      ok.succeed("hostname | tee /dev/stderr | grep '^ok$'")
+      one_two.succeed("hostname | tee /dev/stderr | grep '^onetwo$'")
+      a_b.succeed("hostname | tee /dev/stderr | grep '^a-b$'")
+
+  '';
+}