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

Jump to bottom

nixosTests.containers*: port rest to python #74761

Merged
merged 2 commits into from Dec 10, 2019
Merged

nixosTests.containers*: port rest to python #74761

merged 2 commits into from Dec 10, 2019

Conversation

mmilata
Copy link
Member

@mmilata 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

tfc
tfc reviewed Dec 1, 2019
View reviewed changes
nixos/tests/containers-extra_veth.nix Outdated

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

Choose a reason for hiding this comment

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

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".

tfc
tfc reviewed Dec 1, 2019
View reviewed changes
nixos/tests/containers-extra_veth.nix Outdated
$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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

tfc
tfc reviewed Dec 1, 2019
View reviewed changes
nixos/tests/containers-physical_interfaces.nix Outdated
server.wait_for_unit("default.target")
server.succeed("ip link show dev eth1 >&2")

with subtest("simple physical interface"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
with subtest("simple physical interface"):
with subtest("Simple physical interface is up"):

tfc
tfc reviewed Dec 1, 2019
View reviewed changes
nixos/tests/containers-physical_interfaces.nix Outdated
# 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"):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

tfc
tfc reviewed Dec 1, 2019
View reviewed changes
Copy link
Contributor

@tfc tfc left a comment

Choose a reason for hiding this comment

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

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

@mmilata
Copy link
Member Author

mmilata commented Dec 1, 2019

@tfc fixed, please take a look

@tfc
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
Copy link
Member 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
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
Copy link
Member 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.

flokli
flokli requested changes Dec 2, 2019
View reviewed changes
nixos/tests/containers-extra_veth.nix Outdated

# Destroying a declarative container should fail.
$machine->fail("nixos-container destroy webserver");
DEBUG_OUTPUT = False
Copy link
Contributor

Choose a reason for hiding this comment

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

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

nixos/tests/containers-extra_veth.nix Outdated
DEBUG_OUTPUT = False

machine.wait_for_unit("default.target")
out = machine.succeed("nixos-container list")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@tfc tfc mentioned this pull request Dec 2, 2019
Closed
@flokli
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
Draft
10 tasks
@mmilata
Copy link
Member Author

mmilata commented Dec 8, 2019

@flokli sure, PTAL

flokli
flokli reviewed Dec 10, 2019
View reviewed changes
nixos/tests/containers-extra_veth.nix
import ./make-test.nix ({ pkgs, ...} : {
name = "containers-bridge";
import ./make-test-python.nix ({ pkgs, ...} : {
name = "containers-extra_veth";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch, thanks!

@flokli flokli merged commit b5e53a7 into NixOS:master Dec 10, 2019
@mmilata mmilata deleted the containers-python-test branch December 10, 2019 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli requested changes

@tfc tfc tfc approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mmilata @tfc @flokli