nixosTests.networking: Port tests to python #75721
Conversation
|
This is probably the issue: |
nixos/tests/networking.nix
Outdated
| $router->waitUntilSucceeds("ping -c 1 192.168.3.1"); | ||
| $client->waitUntilSucceeds("ping -c 1 192.168.3.1"); | ||
| router.wait_until_succeeds("ping -c 1 192.168.3.1") | ||
| client.wait_until_succeeds("ping -c 1 192.168.3.1") |
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)| router.wait_until_succeeds("ping -c 1 192.168.2.1") | ||
| router.wait_until_succeeds("ping -c 1 192.168.2.2") | ||
| router.wait_until_succeeds("ping -c 1 fd00:1234:5678:2::1") | ||
| router.wait_until_succeeds("ping -c 1 fd00:1234:5678:2::2") |
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
There was a problem hiding this comment.
I'll stick to #75721 (comment) :)
| router.wait_until_succeeds("ping -c 1 192.168.1.1") | ||
| router.wait_until_succeeds("ping -c 1 192.168.1.2") | ||
| router.wait_until_succeeds("ping -c 1 192.168.1.3") |
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
| client_with_privacy.wait_until_succeeds( | ||
| "ip addr show dev eth1 | grep -q 'fd00:1234:5678:1:'" | ||
| ) | ||
| client.wait_until_succeeds("ip addr show dev eth1 | grep -q 'fd00:1234:5678:1:'") |
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.
There was a problem hiding this comment.
see wait_until_succeeds above.
This is "just" a warning which happens if |
|
@Ma27 Thank you, I saw the commit but after I left the comment. |
kampka
force-pushed
the
port-tests-networking
branch
from
81fab31
to
bfda444
Compare
kampka
force-pushed
the
port-tests-networking
branch
from
bfda444
to
9697ff0
Compare
|
@GrahamcOfBorg test networking.scripted |
kampka
force-pushed
the
port-tests-networking
branch
from
9697ff0
to
98d562f
Compare
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