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

nixpkgs/nixos/lib/testing-python.nix

Line 38 in 85f3d86

black --check --diff ${testDriverScript}

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

• nixpkgs/nixos/tests/installed-tests/default.nix

  Line 25 in 85f3d86

  , preTestScript ? ""

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

• nixpkgs/nixos/tests/gnome3.nix

  Line 28 in 85f3d86

  # Keep line widths somewhat managable

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 #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 #96396 (comment):

@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 #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(<nl>" 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 ***@***.***> wrote: It seems #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: #72964 (comment)

[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 #72964 (comment).

[stale]

I marked this as stale due to inactivity. → More info

[blaggacao]

• I researched and confirm black doesn't seem to allow disable only line lengths.
• I confirm also that the warning with skipLint is triggering false alarm (on me*) and it is also extremly contxt free (Where is linting disabled? Does it hurt?)

* it is a big red warning when passing tests. I immediatly assume that something with the test subject is wrong as opposed to the test instrumentalization, since this is what I'm on the lookout when running tests. My brain immediatly goes in high alert in search for an error in the test subject. It's a context mismatch.

---

Since they appear to be harmful, is removing black checks (entirely) until we come up with a better solution an option?

Please note, this is a trade-off decision. Unfortunately, It currently can't be made pareto-efficient. But the current state is hurtful.

_{If no objection occurs within a few days, I'd feel compelled to assume consensus about a net gain associated with this decision and propose that change.}

[jtojnar]

There was also a suggestion to patch the line length check out (or setting it to infinity or something).

[Ma27]

I confirm also that the warning with skipLint is triggering false alarm (on me)

Can you explain what that means exactly @blaggacao ? :)

[blaggacao]

@Ma27 Updated my post above, thanks for requesting clarification ❤️

@jtojnar

There was also a suggestion to patch the line length check out (or setting it to infinity or something).

I think the one individual preference that at first appears to be offset (by the categories of reasoning in pareto-efficiency terms) through removing black all together is a check for actual code validity.

However that would later be catched by the python interpreter in a much more explicit way I guess?

Traceback...

Vs

Oh No! :brokenheartemjoi: 1 file would be reformatted

Note in the second example it doesn't even mention it's black who's talking. Nor give any additional context. Really confusing / bad UX. I litterally had to search the internet (sic!) for the phrase segment ("file would be reformatted"), which happened to be sufficiently unique to have a hit within the black codebase. Then I searched the testing subtree of nixpkgs/nixos for black and only then did I understand the context. Before that, I was completely mislead / left stranded.

[Ma27]

• it is a big red warning when passing tests. I immediatly assume that something with the test subject is wrong as opposed to the test instrumentalization, since this is what I'm on the lookout when running tests. My brain immediatly goes in high alert in search for an error in the test subject. It's a context mismatch.

Since I'm in fact the one responsible for this, let me explain why it looks the way it does:

• I mainly wanted a flag to quickly turn off linting for writing tests (hence the short, probably suboptimal name) because linting for each new nix-build significantly slowed down my dev cycle. The warning exists to make sure that nobody forgets to remove this before committing to nixpkgs.
• I used lib.warn to print a warning to remain consistent with any other thing in nixpkgs that decides to warn about anything during evaluation. The function is defined in lib/trivial.nix and basically does a builtins.trace and adds the ANSI color code for red in front of the message. So if we want to change the color, we should do it directly there, otherwise it would be inconsistent IMHO.
• Regarding the name: one objective for me is (still) that it has to be rather short. But I agree that without sufficient context it can be kinda confusing. May I ask if you have a better suggestion? At the bottom of https://nixos.org/manual/nixos/stable/#sec-writing-nixos-tests is also a short explanation (but now that I re-read it, it entirely misses the fact that it disables python linting). Apart from that, is there anything else to improve?

[blaggacao]

May I ask if you have a better suggestion?
Apart from that, is there anything else to improve?

Hm, I didn't want to enter that discussion, because sure there always is.

To reasonably limit my deployment of available resources, my quest here really just is about to find out if we can have a "non-vetoed" consensus about a genuine "trade-off"-type decision (non-pareto efficient) about the removal of the black check for their current "harmfulness" (whatever that means, doesn't really matter as long as they cause some sort of "stumbling").

I find it completely fine to veto this, even without reason (of course, reason is always nice). If this is vetoed, I won't assume consensus and won't proceed to make the proposed change in a few days.

(A veto also makes my life easier, since I wouldn't have to follow through on my commitment 😉)

[flokli]

@blaggacao do keep this brief, let me quote my comment from above:

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

Feel free to open a PR for this.

[blaggacao]

Feel free to open a PR for this.

@flokli I'm unable to parse this as either a veto or an absence of veto to my orginal proposed course of action. Can you clarify?

I, unfortunatly, would have to opt for letting go on your proposed alternative course of action. I hope you understand.

[blaggacao]

I'll be desisting for lack of clarity and I don't have a sense clarity is obtained within reasonable effort here.

Don't get me wrong, positions are clear. But not at all what my options to help out with this are.

In such conditions, I don't want to pick this up anymore.

This issue is free to take, again.

[domenkozar]

The minimal change that would get us 80% there is a boolean to switch black off.

[Ma27]

@domenkozar I'm not sure if I'm misunderstanding you, but this is what can be done by setting skipLint = true; in a VM test. As discussed in the PR where I added this, this should not be commited into nixpkgs (I started using it by default for my private VM tests though :P), but it helps at least pretty much for faster development.

@blaggacao I'm confused. First up, I asked for how to improve the semantics of an existing thing, so a purely cosmetic change, nothing too controversial about that, right? Also, let me put it like this, is there anything wrong with the suggestion to patch this away @jtojnar brought up?

[blaggacao]

I chose to clear up the confusion in a PM with the involved parties: I'll report back on the outcome, if any.

[Ma27]

Just wrote an answer. The problem was that I missed your initial suggestion about a full removal of black, also in public, sorry for that! For the record, I'm absolutely not opposed to that, hence definitely no veto from my side.

[flokli]

I think we want some kind of linter, to spot very obvious mistakes, and ensure some consistency in the way tests inside nixpkgs are structured. There's similar efforts to have this for other non-compiled scripts, like piping shellscripts through shellcheck.

So far, the linter is mostly fine for nixpkgs, it can be disabled for out-of-nixpkgs usage where people don't care, or in-nixpkgs during development.

It could be made more ergonomic, by disabling the line length checks in our existing linter, but so far since September 2020 noone bothered enough to tackle this.

I'd personally have huge reservations against simply removing the linter. We should either come up with a more relaxed linter (as the issue title requests), or better docs, so people can use skipLint during development, and only need to fight the slightly-overzealous linter in a single pass at the end.

[blaggacao]

I think we want some kind of linter, to spot very obvious mistakes, and ensure some consistency in the way tests inside nixpkgs are structured.

The current state of technological advancement offers formatters, often times integrated into pre-commit hooks or directly within the IDE on save.

As a general guideline, out of respect for our users, we should probably care not to fall too far behind that "current state of technology".

[roberth]

Please don't conflate formatters and linters. I don't think anyone has a problem with a python linter. It's the formatter that does more harm than good in this case.

state of advancement offers formatters [...] pre-commit [...] on save

Typically these are only capable of formatting a single language. They don't know the language used in string literals and such, and they can't easily call other formatters because the content of the literal may not be valid without running the program.
While it's not impossible to accomplish, it's just not that simple.
If someone wants to integrate formatters in such a way, that would be great, but let's not a hold our breath. After all, we use Nix to solve real problems, not for code to look nice and not to avoid trivial merge conflicts at any cost.

The minimal change that would get us 80% there is a boolean to switch black off.

[blaggacao]

Please don't conflate formatters and linters. I don't think anyone has a problem with a python linter. It's the formatter that does more harm than good in this case.

Oh, I think you probably misread my general opinion as a conflation. That was not the intended message, I'm sorry.

The intended message was: if we do linting, then we probably should care to do it in a comparable UX way as people are accustomed to. That does not mean we must do it, but we should at least care for that aspect.

I think the current situation does not reflect very much of a care for modern UX considerations.

[roberth]

This is such a waste of energy.

#80068 (comment)

[roberth]

#122197

[roberth]

We're switching to an actual linter. Please help correct the findings here #122201 (comment)

Time for action!