-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
nixosTests: Port more tests #78556
nixosTests: Port more tests #78556
Conversation
removed the |
@GrahamcOfBorg test ihatemoney buildbot |
@GrahamcOfBorg test keymap |
bd17460
to
01a0fcc
Compare
@GrahamcOfBorg test ihatemoney keymap buildbot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed
buildbot.nix
ihatemoney.nix
and I'm pretty confident in them.
I did look over the testScript
in keymap.nix
and it looks fine, but the quoting stuff is a bit fuzzy for me. So I think someone more familiar with that specific test should look at it.
I really appreciate the porting effort. But as I already explained in some other "bulk PRs", if this would have been 3 separate PRs, it'd be much easier to just merge the two reviewed ones. |
True. This is how I've been doing my own PRs so they don't get blocked. |
@flokli @worldofpeace Currently it is the most comfortable workflow for me to sit down for an hour before or after work, port 1-3 tests, open a PR for them and this is how it happens. But you're right, one test per PR has mentioned advantages and i will do it like that in the future. |
And we totally appreciate that as well 🎊 |
@flokli you were right, i pythonified the whole thing and it looks a bit more structured now. |
@GrahamcOfBorg test keymap |
@tfc Thanks! This is much more understandable now :-) |
@GrahamcOfBorg test keymap |
Motivation for this change
#72828
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)cc @flokli @worldofpeace