nixosTests.networking: Port tests to python#75721
Conversation
rnhmjoj
left a comment
There was a problem hiding this comment.
Besides the nitpicks it looks good. Thank you for your work!
|
This is probably the issue: |
nixos/tests/networking.nix
Outdated
There was a problem hiding this comment.
why not:
vlan1_ips = [ "192.168.1.1", " 192.168.1.2", " 192.168.1.3", "192.168.1.10" ]
vlan2_ips = [ "192.168.2.1", " 192.168.2.2" ]
gateway_ip = "192.168.3.1"
def ping_until_succeeds(machine, addr):
machine.wait_until_succeeds(f"ping -c 1 {addr}")
with subtest("Test VLAN 1"):
for machine in client, router:
for ip in vlan1_ips:
ping_until_succeeds(machine, ip)
with subtest("Test VLAN 2"):
for machine in client, router:
for ip in vlan2_ips:
ping_until_succeeds(machine, ip)
with subtest("Test default gateway"):
ping_until_succeeds(router, gateway_ip)
ping_until_succeeds(client, gateway_ip)
nixos/tests/networking.nix
Outdated
There was a problem hiding this comment.
this code is rather repetitive and could live with some lists of ips, loops and well-named subprocedures, as in the example before
nixos/tests/networking.nix
Outdated
There was a problem hiding this comment.
how about:
import itertools
def ping_until_succeeds(machine, ip):
machine.wait_until_succeeds(f"ping -c 1 {ip}")
machines = [client1, client2, router]
ips = ["192.168.1.1", "192.168.1.2", "192.168.1.3" ]
with subtest("Test bridging"):
for i in itertools.product(machines, ips):
ping_until_succeeds(*i)There was a problem hiding this comment.
In general I prefer simple, repetetive code in tests as opposed to more complex solutions.
At the moment you can read the test without knowing much about how python works.
Your example makes the code more compact / elegant, but also harder to understand.
I can refactor it if you want, but I don't think we should strive for idiomatic python in these tests.
There was a problem hiding this comment.
Don't see it as a hard requirement. I am fine with either solutions, i just like code to be compact which also reduces the risk for unseen typos
nixos/tests/networking.nix
Outdated
There was a problem hiding this comment.
here, the output of the identical command could again be stored in a variable and then be asserted against with the ip addresses.
Also, the ip addresses could be put into variables with descriptive names, to make this nicer to read.
This is "just" a warning which happens if |
|
@Ma27 Thank you, I saw the commit but after I left the comment. |
81fab31 to
bfda444
Compare
bfda444 to
9697ff0
Compare
|
@GrahamcOfBorg test networking.scripted |
9697ff0 to
98d562f
Compare
rnhmjoj
left a comment
There was a problem hiding this comment.
It looks all good to me.
All the tests (except networkd.virtual) are passing and the error reporting, if I force a failure, is working as expected.
Some tests could be rewritter more concisely but I too agree with @kampka that's better to keep things explicit and use fewer abstractions in tests.
Motivation for this change
#72828
Note that the
networkdversion of thenetworkingtest suite does not succeed.This is not an issue with the port but was already the case in the Perl version of the tests.
The issue is that
networkdhas issues setting up some of the network configurations.Since I am not very familiar with the
networkdsetup, I'd like to address that in a different PR.Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @tfc @rnhmjoj