Skip to content

nixosTests.containers*: port rest to python#74761

Merged
flokli merged 2 commits intoNixOS:masterfrom
mmilata:containers-python-test
Dec 10, 2019
Merged

nixosTests.containers*: port rest to python#74761
flokli merged 2 commits intoNixOS:masterfrom
mmilata:containers-python-test

Conversation

@mmilata
Copy link
Copy Markdown
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

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Dec 1, 2019
tfc
tfc reviewed Dec 1, 2019
View reviewed changes
nixos/tests/containers-extra_veth.nix Outdated
Copy link
Copy Markdown
Contributor

@tfc tfc Dec 1, 2019

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
Copy link
Copy Markdown
Contributor

@tfc tfc Dec 1, 2019

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
Copy link
Copy Markdown
Contributor

@tfc tfc Dec 1, 2019

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
Copy link
Copy Markdown
Contributor

@tfc tfc Dec 1, 2019

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
Copy Markdown
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 mmilata force-pushed the containers-python-test branch from 2ede8fd to e32692f Compare December 1, 2019 15:42
@mmilata
Copy link
Copy Markdown
Member Author

mmilata commented Dec 1, 2019

@tfc fixed, please take a look

@tfc
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 containers-python-test branch from e32692f to 931a305 Compare December 1, 2019 17:32
@mmilata
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member

@flokli flokli Dec 2, 2019

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
Copy link
Copy Markdown
Member

@flokli flokli Dec 2, 2019

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
Copy Markdown
Member Author

@mmilata mmilata Dec 8, 2019

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
Copy Markdown
Member

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 mmilata force-pushed the containers-python-test branch from 931a305 to 6fbb76c Compare December 8, 2019 17:16
@mmilata
Copy link
Copy Markdown
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
Copy Markdown
Member

@flokli flokli Dec 10, 2019

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 Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mmilata @tfc @flokli