nixos/test: colour machine names #96152
Conversation
nixos/lib/test-driver/test-driver.py
Outdated
| ) | ||
|
|
||
|
|
||
| @functools.lru_cache() |
There was a problem hiding this comment.
i find it a bit unsound that a caching mechanism is used on a function that uses side effects.
How about making the Machine constructor pick a color from the iterator and then simply store that for its runtime?
There was a problem hiding this comment.
long term there is the rough plan to use python's logging frame work. a variant where a logger is injected into each machine's constructor would make a lot of sense, as this can be easily combined with coloring of machines.
There was a problem hiding this comment.
@tfc it isn't unsound, lru_cache is documented to grow infinitely if no maximum size is specified. Making it in the machine was my original plan, but it gets messy, as the Machine doesn't control the Logger which
There was a problem hiding this comment.
*which has maybe_prefix, which needs the colour
There was a problem hiding this comment.
I don't have a strong opinion on that that. The method suggested by @tfc sounds a bit easier to understand is is potentially less code. The color formatting could be even inlined in maybe_prefix.
There was a problem hiding this comment.
@Mic92 the serial output doesn't come through maybe_prefix though, it happens directly in Machine.process_serial_output
|
Can we switch to the python logging framework (and by this, making tests more reliable) first, before adding new features? Discussion started here: #86486 (comment) |
|
I really like this, but this can be solved much more elegantly in the formatter. |
|
Is the python logging module even needed? |
|
Yeah sorry I really don't have the context to implement the switch to logging, or arm hardware to debug flakeyness with it... if someone else moves to logging later on they can feel free to pull out the colouring if it's making it hard and I'll reimplement it |
Things still weren't thread-safe, as I wrote in the linked comment.
Agreed, but I'd also be hesitant adding more features into something we end up needing to refactor anyways. Maybe it's best to keep this PR open, until someone picked up the initial refactor. |
|
see #96254 |
|
@JJJollyjim with #96254 merged, would you be up to update this? |
JJJollyjim
force-pushed
the
colour-test-machines-staging
branch
from
9bbcfeb
to
d7875ca
Compare
|
@ofborg test bitwarden.sqlite |
|
oops, that turns out to want to rebuild a lot. @ofborg test boot |
|
ugh, i forget ofborg won't test attrsets @ofborg test qboot |
ofborg
bot
requested review from
blitz,
kalbasit,
saschagrunert,
wucke13,
adisbladis,
zowoq,
cstrahan and
vdemeester
ofborg
bot
added
10.rebuild-darwin: 1-10
and removed
10.rebuild-darwin: 0
10.rebuild-linux: 1
labels
…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>
…-test-machines-staging"" This reverts commit a0a421b.
Add changes from NixOS#96254 and NixOS#96152. Remove `Machine.nested` - use `with subtest("Test")` in tests that used `nested`.
Motivation for this change
This makes NixOS Test output involving many machines a lot easier to visually parse:
The machine-readable XML output is unaffected.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)