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.

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

Merged
merged 16 commits into from Nov 4, 2019
Merged

Conversation

tfc
Copy link
Contributor

@tfc tfc commented Oct 22, 2019

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.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
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):

  • finish porting all funcitonality from the perl driver to python (ocr, coverage, ... it's not much missing)
  • port all tests to python (so far these are roughly ~300 tests)
  • remove perl test driver so there is only one
  • squash all commits with good commit messages (following @jonringer's suggestion)

So far @blitz @jtraue @mdvjd are helping me already

@jonringer
Copy link
Contributor

jonringer commented Oct 22, 2019

Do you mind changing the title and your first commiit to something like nixos: add python testing support

the later 3 commits according to contributing.md should be:

nixos/bittorent: convert test to python
nixos/login: convert test to python
nixos/postgres: convert test to python

@jonringer
Copy link
Contributor

but this is awesome, i think python will enable a lower barrier-to-entry for nixos than Perl.

@tfc
Copy link
Contributor Author

tfc commented Oct 22, 2019

@jonringer I fixed the commit messages according to your suggestions.

@jonringer
Copy link
Contributor

seeing as python support might be a disruptive change, please allow for some time for the necessary communication to happen :)

@tfc tfc changed the title Integration test python nixos: add python testing support Oct 23, 2019
@blitz
Copy link
Contributor

blitz commented Oct 23, 2019

@tfc Super nice! Life is too short to learn Perl. :)

@jonringer
Copy link
Contributor

cc @worldofpeace @FRidh

@FRidh
Copy link
Member

FRidh commented Oct 24, 2019

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.

@tfc
Copy link
Contributor Author

tfc commented Oct 24, 2019

@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.

@blitz
Copy link
Contributor

blitz commented Oct 24, 2019

Maybe an interesting data point: https://trends.google.com/trends/explore?date=today%205-y&geo=US&q=%2Fm%2F05zrn

@worldofpeace
Copy link
Contributor

TBH, I would play with testing much more strongly if it was in python.
I don't think it's too unreasonable to support multiple types of testing scripts.
Can we not have a testing-python.nix file? It's the same as testing.nix at a glance, but just changed to use the python things. So testing.nix needs support to use different testing drivers.

Though I'm realizing you've done this on purpose so as to get feedback.
Currently short on time, but I can give in depth thoughts and opinions soon.

@domenkozar
Copy link
Member

cc @copumpkin you wanted this :D

@tfc
Copy link
Contributor Author

tfc commented Oct 26, 2019

Cc @adisbladis

@tfc
Copy link
Contributor Author

tfc commented Oct 26, 2019

Cc @azazel75

@tfc tfc force-pushed the integration-test-python branch 5 times, most recently from 4b99c89 to ec116fa Compare October 27, 2019 13:23
@jonringer
Copy link
Contributor

@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 :)

@worldofpeace
Copy link
Contributor

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:

So it appears the people you've talked to at nixcon agreed on porting testing completely to python?
Not that I disagree with that, maintaining multiple testing drivers doesn't really sound all that great.

Can the people who discussed this at nixcon publicize their viewpoints here?
Because currently it seems there was an agreement to port testing completely to python and to accept this change, and maybe not everyone agrees with this, or at least the people @tfc had discussion with agreed with it. Mostly looking out for people so they can know why.

If my assumption here is incorrect, correction is welcome.

Copy link
Contributor

@flokli flokli left a 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 flokli merged commit 2290632 into NixOS:master Nov 4, 2019
@worldofpeace
Copy link
Contributor

@flokli Why wasn't https://github.com/NixOS/nixpkgs/pull/71684/files#r340032396 done?
I believe I agreed with @edolstra here #71684 (comment). Is that going to be in another PR?

@flokli
Copy link
Contributor

flokli commented Nov 5, 2019

@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.

@worldofpeace
Copy link
Contributor

@flokli I think I linked the wrong comment https://github.com/NixOS/nixpkgs/pull/71684/files#r340031960

@flokli
Copy link
Contributor

flokli commented Nov 5, 2019

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.

@marijanp marijanp mentioned this pull request Nov 5, 2019
10 tasks
@marijanp marijanp mentioned this pull request Nov 5, 2019
10 tasks
@1000101 1000101 mentioned this pull request Nov 5, 2019
10 tasks
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Nov 5, 2019
nixos: add python testing support
(cherry picked from commit 2290632)
@FRidh
Copy link
Member

FRidh commented Nov 6, 2019

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.

@domenkozar
Copy link
Member

domenkozar commented Nov 6, 2019

But... python does not look an improvement at all, it differs from perl only in syntax,

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.

Even nodejs seems favorable than python.

That could be argued, but it's least designed language of the popular ones we have.

scala + monix,

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.

@FRidh
Copy link
Member

FRidh commented Nov 6, 2019

@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.

@domenkozar
Copy link
Member

Quite a few scripts have been rewritten to python, so I'd rather say: we're replacing perl for python.

@domenkozar
Copy link
Member

domenkozar commented Nov 6, 2019

$ cd nixos
$ git co release-16.03
$ find . -name "*.py" | wc -l
1

$ find . -name "*.pl" | wc -l
8
$ git co master
$ find . -name "*.py" | wc -l
5

$ find . -name "*.pl" | wc -l
7

Where we have yet to remove test-driver.pl some day.

@marijanp marijanp mentioned this pull request Nov 6, 2019
10 tasks
@tfc tfc deleted the integration-test-python branch January 27, 2020 21:00
@aszlig
Copy link
Member

aszlig commented Feb 1, 2020

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 nginx-etag test) and it's no longer the case, but while I would prefer PEP-8 style indenting for standalone Python files, it's not only very weird to look at when used inline - given that Nix expressions commonly use 2 spaces for indentation - but also makes it annoying to tab twice in the editor for every block.

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 black only knows about the resulting script, this would result in an error and you need to write it like this:

{ 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 black would break it, a simple version change of a derivation could break the validation of the test script, eg. if you have somedrv-1.0.99 versus somedrv-1.1.0.

@worldofpeace
Copy link
Contributor

I have the same issues #72964 @aszlig

@tfc
Copy link
Contributor Author

tfc commented Feb 1, 2020

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:
... 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 black would break it, a simple version change of a derivation could break the validation of the test script, eg. if you have somedrv-1.0.99 versus somedrv-1.1.0.

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.
But the problem you just described sounds indeed painful. If we ever run into it in a certain frequency, i could imagine relaxing specific black rules.

@Mic92
Copy link
Member

Mic92 commented Feb 1, 2020

Some tooling would help to automatically format nixos tests using black. This is not trivial though. Especially since strings could contain interpolations from nix.

@aszlig
Copy link
Member

aszlig commented Feb 1, 2020

@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.

flokli added a commit to flokli/nixpkgs that referenced this pull request Feb 9, 2020
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet