Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixosTests.containers*: port rest to python #74761

Merged
merged 2 commits into from Dec 10, 2019

Conversation

@mmilata
Copy link
Contributor

mmilata commented Dec 1, 2019

Motivation for this change

#72828

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @kampfschlaefer @tfc @flokli @worldofpeace

# Status of the webserver container.
$machine->succeed("nixos-container status webserver") =~ /up/ or die;
with subtest("Status of the webserver container"):

This comment has been minimized.

Copy link
@tfc

tfc Dec 1, 2019

Contributor

in failure cases, the subtest title should ideally tell you what the high-level expectations are. While the container will have some kind of status, it is most helpful when the title says "Status of webserver container is UP".

$machine->succeed("ip link show veth1") =~ /master br1/ or die;
with subtest("Ensure the veth1 is part of br1 on the host"):
out = machine.succeed("ip link show veth1")
assert "master br1" in out
# Debug

This comment has been minimized.

Copy link
@tfc

tfc Dec 1, 2019

Contributor

there's a lot to comment out in debug cases. Is it likely that all these prints will help in case this test fails in the future? if so, why not add some constant DEBUG_OUTPUT in the beginning of the test and then put these commands into some conditional - then the debugging user can simply set one variable to True and get it all with less work.

server.wait_for_unit("default.target")
server.succeed("ip link show dev eth1 >&2")
with subtest("simple physical interface"):

This comment has been minimized.

Copy link
@tfc

tfc Dec 1, 2019

Contributor
Suggested change
with subtest("simple physical interface"):
with subtest("Simple physical interface is up"):
# that the device is present in the container.
server.succeed("nixos-container run server -- ip a show dev eth1 >&2")
with subtest("physical device in bridge in container"):

This comment has been minimized.

Copy link
@tfc

tfc Dec 1, 2019

Contributor

"...is up" or "...can be pinged" or something.

Copy link
Contributor

tfc left a comment

Please change the subtest titles in a way that they tell the user more about the high level expectations

@mmilata mmilata force-pushed the mmilata:containers-python-test branch from 2ede8fd to e32692f Dec 1, 2019
@mmilata

This comment has been minimized.

Copy link
Contributor Author

mmilata commented Dec 1, 2019

@tfc fixed, please take a look

@tfc

This comment has been minimized.

Copy link
Contributor

tfc commented Dec 1, 2019

@tfc fixed, please take a look

Thx @mmilata looks awesome! One thing that i did not think of earlier (sorry!) is that tests are even better to read if you bundle multiple success/fail calls that follow up on each other into a single call.

@mmilata

This comment has been minimized.

Copy link
Contributor Author

mmilata commented Dec 1, 2019

@tfc not sure I follow. Like changing this

          machine.succeed("ping -n -c 1 192.168.0.100")
          machine.succeed("ping -n -c 1 fc00::2")

into

          machine.succeed("ping -n -c 1 192.168.0.100 && ping -n -c 1 fc00::2")

?

@tfc

This comment has been minimized.

Copy link
Contributor

tfc commented Dec 1, 2019

@mmilata more like:

machine.succeed(
    "ping -n -c 1 192.168.0.100",
    "ping -n -c 1 fc00::2"
)
@mmilata mmilata force-pushed the mmilata:containers-python-test branch from e32692f to 931a305 Dec 1, 2019
@mmilata

This comment has been minimized.

Copy link
Contributor Author

mmilata commented Dec 1, 2019

@tfc oh, I had no idea succeed accepts varargs. I've changed some of the calls, left multiple calls in others (e.g. your example) because the formatter wants me to have both commands on single line which I think is worse for readability.

@tfc
tfc approved these changes Dec 2, 2019
# Destroying a declarative container should fail.
$machine->fail("nixos-container destroy webserver");
DEBUG_OUTPUT = False

This comment has been minimized.

Copy link
@flokli

flokli Dec 2, 2019

Contributor

This (and the conditionals further down) should probably go away.

DEBUG_OUTPUT = False
machine.wait_for_unit("default.target")
out = machine.succeed("nixos-container list")

This comment has been minimized.

Copy link
@flokli

flokli Dec 2, 2019

Contributor

I'd better call it output, out is too similar to $out ;-)

This comment has been minimized.

Copy link
@mmilata

mmilata Dec 8, 2019

Author Contributor

I've changed this to assert "substr" in machine.succeed("cmd") which seems to be widely used.

@flokli

This comment has been minimized.

Copy link
Contributor

flokli commented Dec 7, 2019

@mmilata could you adress the requested changes? Would be nice to merge this in :-)

@flokli flokli mentioned this pull request Dec 7, 2019
4 of 10 tasks complete
@mmilata mmilata force-pushed the mmilata:containers-python-test branch from 931a305 to 6fbb76c Dec 8, 2019
@mmilata

This comment has been minimized.

Copy link
Contributor Author

mmilata commented Dec 8, 2019

@flokli sure, PTAL

import ./make-test.nix ({ pkgs, ...} : {
name = "containers-bridge";
import ./make-test-python.nix ({ pkgs, ...} : {
name = "containers-extra_veth";

This comment has been minimized.

Copy link
@flokli

flokli Dec 10, 2019

Contributor

Nice catch, thanks!

@flokli flokli merged commit b5e53a7 into NixOS:master Dec 10, 2019
1 check was pending
1 check was pending
grahamcofborg-eval Checking out master
Details
@mmilata mmilata deleted the mmilata:containers-python-test branch Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.