-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
nixos: add python testing support #71684
Conversation
Do you mind changing the title and your first commiit to something like the later 3 commits according to contributing.md should be:
|
but this is awesome, i think python will enable a lower barrier-to-entry for nixos than Perl. |
4e8b968
to
33912f3
Compare
@jonringer I fixed the commit messages according to your suggestions. |
seeing as python support might be a disruptive change, please allow for some time for the necessary communication to happen :) |
33912f3
to
cc9d974
Compare
@tfc Super nice! Life is too short to learn Perl. :) |
I haven't written any NixOS tests myself, but is the language (Perl) such a barrier that we need to implement it (also) in Python? From what I understand the tests are often quite simple, not requiring any deep knowledge of Perl. Personally I don't mind having it in Python at all, but let's keep this open for a while so those that use the NixOS testing framework can review. Also, it seems to be we need a code owner for the original parts and the newly added parts. |
@FRidh perl is not a barrier to start writing simple nixos integration tests at all. Once you get used to them you would like to write fancier and fancier tests that use the language's facilities to reduce duplication and to make things a bit more dynamic, and maybe also add libraries (currently not supported in my proposal yet, but i also don't want to blow up the PR). That is where non-Perl users would like to use Python. If an owner is needed for the Python part, i would volunteer as this would be heavily used by my colleagues at work. |
Maybe an interesting data point: https://trends.google.com/trends/explore?date=today%205-y&geo=US&q=%2Fm%2F05zrn |
TBH, I would play with testing much more strongly if it was in python. Though I'm realizing you've done this on purpose so as to get feedback. |
cc @copumpkin you wanted this :D |
Cc @adisbladis |
Cc @azazel75 |
cc9d974
to
5ef0075
Compare
4b99c89
to
ec116fa
Compare
@tfc I would hold off on converting a lot of the tests over to python. It will expand how much this PR is changing and make it harder to come to a decision on whether or not it should be merged. Even though a lot of the converted tests seem to be a 1:1 translation :) |
So it appears the people you've talked to at nixcon agreed on porting testing completely to python? Can the people who discussed this at nixcon publicize their viewpoints here? If my assumption here is incorrect, correction is welcome. |
fcd4187
to
40396a7
Compare
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.
Thanks a lot for this!
@flokli Why wasn't https://github.com/NixOS/nixpkgs/pull/71684/files#r340032396 done? |
@worldofpeace there was no clear consensus on #71684 (comment), and as this can always be done in a follow-up PR, I didn't want to stall this any further. |
@flokli I think I linked the wrong comment https://github.com/NixOS/nixpkgs/pull/71684/files#r340031960 |
In that case, porting isn't finished, but I didn't consider this a requirement for this PR, which is why I asked about #71684 (comment). I assume porting the remaining nixos vm tests will happen soon, and once that's done, we can remove the perl test (and maybe rename the python one to the original location). Docs already only document the python one. |
nixos: add python testing support (cherry picked from commit 2290632)
Without going into whether Python is an improvement or not, but in these cases we should just write the data as JSON and read that, regardless of language. That means currently explicitly writing to JSON and in the future with structured attributes we can simply pass the dict as an attribute. |
That's untrue, the major difference is that nowadays Python is on the top of the list of popular languages, so probably millions more are capable of contributing easily.
That could be argued, but it's least designed language of the popular ones we have.
Well scala is used in very specific settings, so that way I'd actually prefer perl since it's more approachable. I think you're forgetting that any NixOS developer should be able to write these tests. |
@volth if you would like to discuss the future of the stdenv (language), please open a separate issue. I am sure others (including myself) would be interested in discussing it. |
Quite a few scripts have been rewritten to python, so I'd rather say: we're replacing |
Where we have yet to remove |
Maybe it's a bit late, but just wanted to throw in my two cents on this (since I'm buried in other things lately :-/): I do like the switch, since I'm already using Python in a lot of my tests, particularly the WebDriver/Selenium ones, but I find the 4 space indentation and line length enforcement pretty awkward. Maybe I'm not up-to-date on this (last checked was when writing the Same goes for the line length, I'm theoretically fine with the limit and I do enforce 79 chars myself when writing Nix code, but this falls short if you have things like this: { testScript = ''
machine.succeed("ls ${someStorePath}")
'';
} Since { testScript = ''
machine.succeed(
"ls ${someStorePath}"
)
'';
} ... even though the line length is fine on the Nix side. Worse off, if you have a line where the derivation name is just long enough so |
There is generally a flag to turn it off for tests. https://github.com/NixOS/nixpkgs/blob/master/nixos/lib/testing-python.nix#L99 I found black relatively "patronizing" during development, but in the end it gives all python code a "uniform" look and i find that pretty appealing while reading code. |
Some tooling would help to automatically format nixos tests using black. This is not trivial though. Especially since strings could contain interpolations from nix. |
@tfc: Well as mentioned, I do like having an "uniform" look, but for inline code it find the disparity between the indentation levels pretty weird to look at. For the line length issue (or maybe even the indentation issue), I think #72964 would be a better place to discuss. Addendum: I know that there is a flag to turn it off, but if I'd need to do that for every test contributed to nixpkgs, I wonder what benefits there are for the linter/formatter at all. |
Most VM tests have been migrated to use the python test driver (introduced in NixOS#71684), the migration is tracked in NixOS#72828 (which also thankfully uncovered and fixed many currently broken tests) While increasing the acceptance and adoption of NixOS integration tests by using a more popular language, there was also nobody willing to do larger refactors in the currently very convoluted test infrastructure. We plan to remove the perl infrastructure between the 20.03 and 20.09 release, to be able to do these refactorings. Some people might be using Perl tests in their internal CI, so print a warning for 20.03, and give users time to move to the python testing infrastructure.
Motivation for this change
Myself and most people i work with don't know Perl but Python. NixOS integration tests are totally awesome and more people should use/write such to test software.
I would like to support the acceptance and adoption of NixOS integration tests by letting users choose the more popular language Python to implement the imperative integration test part.
Things done
I ported the test driver to python and forked the nix scripts. Of course the duplication can be reduced, but i did not want to put more effort into this in case this work is not accepted in this repo for some reason, so i am eager to hear/read your thoughts.
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @edolstra @thefloweringash @abbradar @samueldr @srhb
Community discussion
After discussing this with some people at nixcon (including @edolstra @Lassulus @adisbladis @domenkozar @garbas ...) i arrived at the following TODO-list to make the whole thing merge-acceptable (Note that there was no decision):
So far @blitz @jtraue @mdvjd are helping me already