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

Port tests to Python #72810

Merged
merged 6 commits into from Nov 5, 2019
Merged

Port tests to Python #72810

merged 6 commits into from Nov 5, 2019

Conversation

marijanp
Copy link
Contributor

@marijanp marijanp commented Nov 5, 2019

Motivation for this change

Now that PR #71684 (nixos: add python testing support) by @tfc got merged, I would like to port some/all of the tests to Python.

Things done

Ported the copy_file_from_host function to Python.
Ported several tests to Python, which also conform the Black code style.
Tests have been run and all of them succeed.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@@ -642,6 +642,14 @@ def wait_for_x(self):
if status == 0:
return

def copy_file_from_host(self, source, target):
"""Copies the file located at source path to the target path.
Note: "source" is equivalent to "from", "target" is equivalent to "to" in the Perl script
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments that reference the perl code lose their relevance as soon as we ported them all and deleted the perl code.

Copy link
Contributor

@flokli flokli Nov 5, 2019

Choose a reason for hiding this comment

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

I'm not sure if we should encourage copying files from the host in general (see my later comment). This might not work with binary files, too.
We should inline the self.succeed call in places where we really have to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moreover this function is only used in the openssh test, so I'll remove the change completely.

server_lazy.copy_file_from_host("key.pub", "/root/.ssh/authorized_keys")

client.succeed("mkdir -m 700 /root/.ssh")
client.copy_file_from_host("key", "/root/.ssh/id_ed25519")
Copy link
Contributor

Choose a reason for hiding this comment

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

client.succeed should return the command output. Could you try generating the key on the client, read the pubkey in the test script, and copy it to the server that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've pushed the changes.

$client->succeed("echo GETINFO version | nc 127.0.0.1 9051") =~ /514 Authentication required./ or die;
client.wait_for_unit("tor.service")
client.wait_for_open_port(9051)
if not "514 Authentication required." in client.succeed(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use assert here?

assert "514 Authentication required." in client.succeed("echo GETINFO version | nc 127.0.0.1 9051")

and it should raise an AssertionError?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, foo or die idiom should be rewritten to assert foo

@FRidh FRidh added this to the 20.03 milestone Nov 5, 2019
@flokli
Copy link
Contributor

flokli commented Nov 5, 2019

LGTM. Do you want to port more tests in here, or should we merge this in?

@tfc
Copy link
Contributor

tfc commented Nov 5, 2019

@flokli many many more are following... i suggest merging early so the PRs don't get too long. Also, this avoids rebasing conflicts and other hassle.

@flokli flokli merged commit 54c0ac5 into NixOS:master Nov 5, 2019
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Nov 5, 2019
Port tests to Python

(cherry picked from commit 54c0ac5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants