Nixpkgs: Relax code style checks in python testing driver's testScripts's

[worldofpeace]

Describe the bug
Currently the python testing driver runs this check on each test script
https://github.com/NixOS/nixpkgs/blob/85f3d86bea70fe0d76a7e3520966c58604f8e5e9/nixos/lib/testing-python.nix#L38

And this is great because it ensures a good code style within the testing scripts.
But unfortunately it isn't as straightforward as this, I can think of couple cases already where this can cause headaches for people

• generated testing scripts/injected

I wrote

• https://github.com/NixOS/nixpkgs/blob/85f3d86bea70fe0d76a7e3520966c58604f8e5e9/nixos/tests/installed-tests/default.nix#L25

recently and it can error out on the final testing script, but that is already abstracted by this function so it doesn't really matter. I had to make special measure to try to appease black.

• nix store paths/nix values

We used nix to keep line width manageable in this test

• https://github.com/NixOS/nixpkgs/blob/85f3d86bea70fe0d76a7e3520966c58604f8e5e9/nixos/tests/gnome3.nix#L28

but if I were to port this to python I'd need to format the code like

machine.wait_until_succeeds(
    "su - alice -c '${startingUp} | grep -q true,..false'"
)

because those values get interpolated and then became really long.
Same if you use a nix store path attribute anywhere, it'll likely error out on the line being too long.

Solutions
We need to relax a lot of black's settings.
@adisbladis pasted these, as to what they're using personally:

• http://dpaste.com/1MPTRKR
• http://dpaste.com/2TW6V8N

And for sure relax string literal lengths to be larger, because of the reasons I mentioned.

[worldofpeace]

Also, what if my test script isn't ready to be committed and I just want to run the vm.
You can't, you have to go and make sure your comments are lined up correctly.
Perhaps there should be a way to disable this, or maybe we should rethink this entirely.

[FRidh]

Somewhat related PR https://github.com/NixOS/nixpkgs/pull/73241.

Black is a formatter, not a linter. It will format it according to a certain spec. Contrary to yapf, it has no options besides line length. If you give it a length of say 1000 for the test scripts, then they will get messy if they include function definitions with many parameters and long parameter names.

Maybe we should avoid string interpolation, and instead write all configuration to a JSON file and let the test script load the relevant variables from a dictionary. In the future that JSON file could be .attrs.json once we use structured attributes. Downside of this approach is that, right now, you could generate your test script and see the exact values at the place of interest. Of course, by using variables you won't have that.

And indeed, maybe we should not use Black for the test scripts at all, and only for library code (the driver).

[adisbladis]

And indeed, maybe we should not use Black for the test scripts at all, and only for library code (the driver).

I'm in strong favour of this approach. Sacrificing string interpolation is the wrong trade-off imho.
We could use a simple linter on the tests themselves and a formatter on the driver.

[khumba]

I think it's important to provide a way to fully disable style checks _and style warnings_ for test scripts, at least for use outside of Nixpkgs. We use NixOS tests at work and it's not important for those scripts to conform to the style rules of NixOS's tests. The warnings during evaluation are just noise.

Could we possibly support skipLint = "silent" or something please? _(Edit: If checking of the test scripts is kept at all, of course!)_

[aszlig]

Here are a few more examples of tests that are going to fail randomly when ported to Python:

• https://github.com/headcounter/deployment/blob/c74d9b63a4aa7c0c91b6f0aca1c72b6db3e91b1b/tests/headcounter/per-vhost/xmppoke.nix#L94-L95
• https://github.com/headcounter/deployment/blob/c74d9b63a4aa7c0c91b6f0aca1c72b6db3e91b1b/tests/nsd-zone-writer.nix#L56
• https://github.com/aszlig/avonc/blob/9c9a3fc0b3ae6277f9f1ca69bcc1566f2aac2e0a/tests/upgrade.nix#L147
• https://github.com/headcounter/shabitica/blob/b5bd254a30b54ec06a363e76273b01d849383809/tests/migrations/default.nix#L110
• https://github.com/headcounter/shabitica/blob/b5bd254a30b54ec06a363e76273b01d849383809/tests/imageproxy.nix#L148
• https://github.com/headcounter/shabitica/blob/b5bd254a30b54ec06a363e76273b01d849383809/tests/backup.nix#L60
• https://github.com/openlab-aux/vuizvui/blob/ba4d305738ea9fef5d9ca87571938f71382062ff/tests/games/starbound.nix#L79

The reason for this is that they're parametrized (eg. via the FQDN or subtest description) and black doesn't know about Nix string antiquotations.

Furthermore, I think using black on inline code also makes the code less readable, because it tends to fill more vertical space (eg. see the Chromium test before and after the conversion).

edit: One workaround would be to start every testScript with # fmt: off by default, so if we just document this accordingly that would be ugly but still work.

[flokli]

For testScript, could we use a code linter variant that behaves similar to black, except it ignores line lengths?

[flokli]

Moved over from https://github.com/NixOS/nixpkgs/pull/96396#issuecomment-682965857:

@Ma27:

I don't really see how this is related to removing a piece of code unused in nixpkgs, and announced to be removed half a year ago.

Well, there are people who also use one of the test frameworks for their own stuff and dislike this "feature" ;-)

You mean, it would be desirable to disable the linter without showing a warning for tests outside nixpkgs, that use the testing framework as a "library"? Or is it only the line lengths parts of the linter that are annoying?

I think having some sort of linting (probably not as strict as now) at least for everything in nixpkgs is desirable.

[Ma27]

You mean, it would be desirable to disable the linter without showing a warning for tests outside nixpkgs

Well, the thing is, it's not a warning, it's an error.

Please note though that I have a skipLint = true; in my VM-test snippet in $EDITOR and I fix errors before committing (if needed), I mainly tried to explain why this is kinda related to the removal of the perl driver).

Or is it only the line lengths parts of the linter that are annoying?

The second thing is the quote-linting. When I do some double- vs single-quote hackery I'm not that happy if the linter asks me to switch quotes because it's "simpler" (it isn't because a contributor has to fixup a small nitpick).

[flokli]

You mean, it would be desirable to disable the linter without showing a warning for tests outside nixpkgs

Well, the thing is, it's not a warning, it's an error.

Huh? If you instantiate the test with skipLint = true, you get a warning that linting is disabled, but the test runs.

Please note though that I have a skipLint = true; in my VM-test snippet in $EDITOR and I fix errors before committing (if needed), I mainly tried to explain why this is kinda related to the removal of the perl driver).

Or is it only the line lengths parts of the linter that are annoying?

The second thing is the quote-linting. When I do some double- vs single-quote hackery I'm not that happy if the linter asks me to switch quotes because it's "simpler" (it isn't because a contributor has to fixup a small nitpick).

Okay, so if we use a linter that doesn't complain about line lengths and doesn't suggest switching to "simpler" quotes, that'd be okay?

[Ma27]

Huh? If you instantiate the test with skipLint = true, you get a warning that linting is disabled, but the test runs.

Right, I guess I misunderstood your previous comment. I brought this up because folks who raised concerns in the perl-removal ticket didn't seem to be aware of this.

Okay, so if we use a linter that doesn't complain about line lengths and doesn't suggest switching to "simpler" quotes, that'd be okay?

Yes IMHO.

[flokli]

It seems https://github.com/NixOS/nixpkgs/pull/96515 from @khumba was closed due to black supporting # fmt: off.

@Ma27 I assume best plan forward would be proposing a PR switching from black to another linter, configured as described above.

[khumba]

I'd missed aszlig's mention that of "fmt: off" earlier, and for my main use case, tests outside of Nixpkgs, that is enough (and yes let's document that). It's funny though, it doesn't seem to disable it completely, Black still complains on extra newlines at EOF, which you get if you use

''
  # fmt: off
  ${testScript}
''

to disable it en masse.

For tests in Nixpkgs, if we keep Black then I'm fine if we just want to relax certain rules. One other rule I thought was unnecessary in some tests I wrote (non-Nixpkgs) was wanting to break the start of a multiline function call "log.log(machine.success(" into two lines with extra indentation just for that part. Feel free to disagree there though, I don't know PEP-8 very well.

On September 5, 2020 7:00:41 a.m. PDT, Florian Klink [e-mail] wrote:

It seems https://github.com/NixOS/nixpkgs/pull/96515 from @khumba was
closed due to black supporting # fmt: off.

@Ma27 I assume best plan forward would be proposing a PR switching from
black to another linter, configured as described above.

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/NixOS/nixpkgs/issues/72964#issuecomment-687615419

[flokli]

@khumba black itself isn't configurable, except turning it off entirely for a certain indentation level, and everything nested deeper (which might explain the errors you're seeing) - which is why I proposed switching to something else, that can be configured in https://github.com/NixOS/nixpkgs/issues/72964#issuecomment-687615419.