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.

Sign up for GitHub

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

nixos/test-driver: use pythons logging module #96254

Merged
merged 4 commits into from Aug 26, 2020
Merged

nixos/test-driver: use pythons logging module #96254

merged 4 commits into from Aug 26, 2020

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Aug 25, 2020 •
edited

Motivation for this change
Things done
  • 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 nixpkgs-review --run "nixpkgs-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.

@Mic92
nixos/test-driver: introduce main method
c1667f8 
This way we not accidentally use introduce/use global variables.
Also it explictly mark the code for the mypy type checker.
@Mic92 Mic92 requested a review from tfc as a code owner August 25, 2020 08:39
@Mic92
Copy link
Member Author

Mic92 commented Aug 25, 2020

@GrahamcOfBorg test ferm

@Mic92 Mic92 changed the title Logging nixos/test-driver: use pythons logging module Aug 25, 2020
@Mic92 Mic92 force-pushed the logging branch 3 times, most recently from edba5a7 to 99f8942 Compare August 25, 2020 09:06
This was referenced Aug 25, 2020
Merged
Merged
@Mic92
nixos/test-driver: switch to pythons' logging lib
392415c 
- Less code
- more thread-safe according to @flokli
@tfc tfc requested a review from flokli August 25, 2020 09:31
@JJJollyjim
Copy link
Member

Haha, I just opened github to open the same PR

@JJJollyjim
Copy link
Member

Here's mine for comparison: https://github.com/JJJollyjim/nixpkgs/tree/python-logging

@JJJollyjim
Copy link
Member

btw, as-is this will break a few tests which were depending on log functions:

2020-08-25-222342_838x934_scrot

@JJJollyjim
Copy link
Member

JJJollyjim commented Aug 25, 2020 •
edited

Personally, I'd like to see automatic (streaming) output to the logger from .succeed and related functions, which would remove the need for most of these log statements (and generally make test development easier in some cases)

(not saying that's needed for this PR, of course)

@Mic92
nixos/test-driver: re-introduce log()
87214db 
Appearantly this is used in tests
@Mic92
Copy link
Member Author

Mic92 commented Aug 25, 2020

@GrahamcOfBorg test nsd

@Mic92
Copy link
Member Author

Mic92 commented Aug 25, 2020

I re-added the log() function.

@JJJollyjim
Copy link
Member

lgtm :)

@Mic92 Mic92 merged commit 4fc7085 into NixOS:master Aug 26, 2020
@Mic92 Mic92 deleted the logging branch August 26, 2020 18:45
@flokli
Copy link
Contributor

flokli commented Aug 26, 2020

Thanks for this! :-)

@tfc tfc mentioned this pull request Aug 26, 2020
Closed
This was referenced Aug 28, 2020
Closed
Merged
andersk
andersk reviewed Aug 30, 2020
View reviewed changes
nixos/lib/test-driver/test-driver.py
Comment on lines -298 to -301
def nested(self, msg: str, attrs: Dict[str, str] = {}) -> _GeneratorContextManager:
my_attrs = {"machine": self.name}
my_attrs.update(attrs)
return self.logger.nested(msg, my_attrs)
Copy link
Contributor

@andersk andersk Aug 30, 2020 •
edited

Choose a reason for hiding this comment

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

Removing Machine.nested has broken a number of tests that used it, including the channel-blocking chromium test:

$ git grep '\.nested' nixos/tests
nixos/tests/chromium.nix:        with machine.nested("Creating a new Chromium window"):
nixos/tests/chromium.nix:        with machine.nested("Waiting for new Chromium window to appear"):
nixos/tests/chromium.nix:        with machine.nested(description):
nixos/tests/dhparams.nix:      with machine.nested("switch to generation ${toString gen}"):
nixos/tests/dhparams.nix:        with machine.nested(f"check bit size of {path}"):
nixos/tests/initrd-network-ssh/default.nix:    with client.nested("waiting for SSH server to come up"):
nixos/tests/taskserver.nix:            with client.nested(f"initialize client for user {user}"):
nixos/tests/taskserver.nix:                with client.nested("importing taskwarrior configuration"):
nixos/tests/taskserver.nix:        with server.nested("(re-)add imperative user bar"):
nixos/tests/virtualbox.nix:      with machine.nested("Checking for privilege escalation"):
nixos/tests/virtualbox.nix:    with machine.nested("Checking for privilege escalation"):

#96699

@andersk andersk mentioned this pull request Aug 30, 2020
Closed
andersk added a commit to andersk/nixpkgs that referenced this pull request Aug 30, 2020
@andersk
Revert "Merge pull request NixOS#96152 from JJJollyjim/colour-test-ma…
a0a421b 
…chines-staging"

This reverts commit 1bff6fe, reversing
changes made to 2995fa4.

There’s presumably nothing wrong with this PR, except that it
conflicts with reverting NixOS#96254 which broke several tests (NixOS#96699).

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
andersk added a commit to andersk/nixpkgs that referenced this pull request Aug 30, 2020
@andersk
Revert "Merge pull request NixOS#96254 from Mic92/logging"
59b6664 
This reverts commit 4fc7085, reversing
changes made to 0e54f3a.

Fixes NixOS#96699.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk andersk mentioned this pull request Aug 30, 2020
Merged
10 tasks
worldofpeace added a commit that referenced this pull request Aug 30, 2020
@worldofpeace
Merge pull request #96703 from andersk/revert-test-logging
5c67236 
Revert “nixos/test-driver: use pythons logging module” (#96254)
@andersk
Copy link
Contributor

andersk commented Aug 30, 2020

This was reverted for now in #96703 to fix #96699.

JJJollyjim added a commit to JJJollyjim/nixpkgs that referenced this pull request Sep 23, 2020
@JJJollyjim
Revert "Revert "Merge pull request NixOS#96254 from Mic92/logging""
5f64c06 
This reverts commit 59b6664.
am-on added a commit to marijanp/nixpkgs that referenced this pull request Dec 8, 2021
@am-on
nixos/test-driver: Use python's native logging
3207452 
Add changes from NixOS#96254 and
NixOS#96152.

Remove `Machine.nested` - use `with subtest("Test")` in tests that used
`nested`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andersk andersk andersk left review comments

@tfc tfc tfc requested changes

@JJJollyjim JJJollyjim JJJollyjim left review comments

@flokli flokli Awaiting requested review from flokli

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@Mic92 @JJJollyjim @flokli @andersk @tfc