Skip to content

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

Merged
Mic92 merged 4 commits intoNixOS:masterfrom
Mic92:logging
Aug 26, 2020
Merged

nixos/test-driver: use pythons logging module#96254
Mic92 merged 4 commits intoNixOS:masterfrom
Mic92:logging

Conversation

@Mic92
Copy link
Copy Markdown
Member

@Mic92 Mic92 commented Aug 25, 2020 •
edited
Loading

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
Copy Markdown
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
tfc
tfc reviewed Aug 25, 2020
View reviewed changes
tfc
tfc reviewed Aug 25, 2020
View reviewed changes
tfc
tfc reviewed Aug 25, 2020
View reviewed changes
@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
@ofborg ofborg bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Aug 25, 2020
tfc
tfc requested changes Aug 25, 2020
View reviewed changes
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Aug 25, 2020
@JJJollyjim
Copy link
Copy Markdown
Member

JJJollyjim commented Aug 25, 2020

Haha, I just opened github to open the same PR

@JJJollyjim
Copy link
Copy Markdown
Member

JJJollyjim commented Aug 25, 2020

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

@JJJollyjim
Copy link
Copy Markdown
Member

JJJollyjim commented Aug 25, 2020

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

2020-08-25-222342_838x934_scrot

@JJJollyjim
Copy link
Copy Markdown
Member

JJJollyjim commented Aug 25, 2020 •
edited
Loading

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

Mic92 commented Aug 25, 2020

@GrahamcOfBorg test nsd

@Mic92
Copy link
Copy Markdown
Member Author

Mic92 commented Aug 25, 2020

I re-added the log() function.

@JJJollyjim
Copy link
Copy Markdown
Member

JJJollyjim commented Aug 26, 2020

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

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

@andersk andersk Aug 30, 2020 •
edited
Loading

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

@tfc tfc tfc requested changes

@flokli flokli Awaiting requested review from flokli

+2 more reviewers

@andersk andersk andersk left review comments

@JJJollyjim JJJollyjim JJJollyjim left review comments

Reviewers whose approvals may not affect merge requirements

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: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Mic92 @JJJollyjim @flokli @andersk @tfc