nixos/testing: Switch from black to pyflakes #122201
Conversation
|
I was working on flake8 and I wrote some code to build the scripts only. See #122204 Do you think pyflakes is better than flake8? Or perhaps we should have both? |
From https://gitlab.com/pycqa/flake8/-/blob/master/README.rst#flake8:
The Also, I highly doubt that |
|
Ok. I was disabling individual style rules, which is not great. I agree about complexity measures. Let's go with pyflakes. |
|
I've added some processing to define the variables that are defined externally. At least sufficient for the |
|
First batch, up to and including @GrahamcOfBorg test _3proxy acme agda airsonic amazon-init-shell ammonite atd avahi avahi-with-resolved awscli |
|
@GrahamcOfBorg test babeld bat bazarr bcachefs beanstalkd bees bind bitcoind bittorrent bitwarden blockbook-frontend boot boot-stage1 borgbackup buildbot buildkite-agents |
|
Ok, ofborg was a bad idea for this PR. Still need to watch out for tests in nested attrs though. |
nixos/lib/testing-python.nix
Outdated
| echo "def testScript(subtest, start_all, ${lib.concatStringsSep ", " nodeNames}):" | ||
| sed -e 's/^/ /' -e 's/^ $//' <$out/test-script | ||
| ) >test-script | ||
| ${python3Packages.pyflakes}/bin/pyflakes test-script |
There was a problem hiding this comment.
Can we move the actual linting into build that is detached from the rest? It is gaining a lot of complexity (comparatively).
Probably good to have a build for the script that passes if linting succeeded (unless disabled?). Having an attribute to tests that disable the whole formatting story could be really useful for debugging. We can probably enforce proper formatting for nixpkgs.
There was a problem hiding this comment.
It's already possible to run only the linting
nix-build -A nixosTests.acme.driver
If that's all you want, it's done in this PR (or #122204 which is cherry-picked here).
If you want to have to attributes, one for running the test and one for linting, that's a new requirement that I would prefer to leave out of scope for now.
Prefixes tested
The command to run is as follows. Replace If you don't have privileges on the nixpkgs repo, leave a comment of your own instead.
nested attributesinstructions tbd
|
|
@roberth: Hydra jobset: https://headcounter.org/hydra/jobset/aszlig/nixos-tests |
That doesn't quite align with the goal of fixing the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
FYI: I've changed the Hydra jobset to only evaluate the |
There was a problem hiding this comment.
Don't merge yet, while aszlig is doing some final touches.
General remarks
- We improved over a dozen of test scripts
- Most tests already passed linting, so this linter can hardly be a burden
Testing
With our manual testing, we should have covered all test scripts. Some test scripts were not testable because of failed dependencies or evaluation errors. This is ok, because the goal is to prevent new bitrot and avoid surprises and small chores where possible.
Conclusion
This PR achieves the goal of #72964 with minimal disruption.
So far, we have used "black" for formatting the test code, which is rather strict and opinionated and when used inline in Nix expressions it creates all sorts of trouble. One of the main annoyances is that when using strings coming from Nix expressions (eg. store paths or option definitions from NixOS modules), completely unrelated changes could cause tests to fail, since eg. black wants lines to be broken. Another downside of enforcing a certain kind of formatting is that it makes the Nix expression code inconsistent because we're mixing two spaces of indentation (common in nixpkgs) with four spaces of indentation as defined in PEP-8. While this is perfectly fine for standalone Python files, it really looks ugly and inconsistent IMO when used within Nix strings. What we actually want though is a linter that catches problems early on before actually running the test, because this is *actually* helping in development because running the actual VM test takes much longer. This is the reason why I switched from black to pyflakes, because the latter actually has useful checks, eg. usage of undefined variables, invalid format arguments, duplicate arguments, shadowed loop vars and more. Signed-off-by: aszlig <aszlig@nix.build> Closes: NixOS#72964
(cherry picked from commit a2c9220568648b4528154ebd8e657add243ed0b4)
Our test driver exposes a bunch of variables and functions, which pyflakes doesn't recognise by default because it assumes that the test script is executed standalone. In reality however the test driver script is using exec() on the testScript. Fortunately pyflakes has $PYFLAKES_BUILTINS, which are the attributes that are globally available on all modules to be checked. Since we only have one module, using this environment variable is fine as opposed to my first approach to this, which tried to use the unstable internal API of pyflakes. The attributes are gathered by the main derivation of the test driver, because we don't want to end up defining a new attribute in the test driver module just to being confused why using it in a test will result in an error. Another way we could have gathered these attributes would be in mkDriver, which is where the linting takes place. However, we do have a different set of Python dependencies in scope and duplicating these will again just cause confusion over having it at one location only. Signed-off-by: aszlig <aszlig@nix.build> Co-Authored-By: aszlig <aszlig@nix.build>
Note: I didn't execute it entirely because I'd have to build chromium for this, but the diff appears fine.
The new linter basically does
def testScript
# ...
before calling `pyflakes`. As this test-script is empty, it would lead
to a syntax-error unless `pass` is added.
Linter error was: f-string is missing placeholders Signed-off-by: aszlig <aszlig@nix.build>
Linter error: use ==/!= to compare constant literals (str, bytes, int, float, tuple) Signed-off-by: aszlig <aszlig@nix.build>
Signed-off-by: aszlig <aszlig@nix.build>
Linter errors reported: 6:32 f-string is missing placeholders 7:26 f-string is missing placeholders 8:32 f-string is missing placeholders 30:32 f-string is missing placeholders 31:26 f-string is missing placeholders 32:32 f-string is missing placeholders 48:32 f-string is missing placeholders 49:26 f-string is missing placeholders 50:32 f-string is missing placeholders 76:32 f-string is missing placeholders 77:26 f-string is missing placeholders 78:32 f-string is missing placeholders Signed-off-by: aszlig <aszlig@nix.build>
Signed-off-by: aszlig <aszlig@nix.build>
There were a bunch of unnecessary f-strings in there and I also removed the "# fmt: on/off" comments, because we no longer use Black and thus won't need those comments anymore. Signed-off-by: aszlig <aszlig@nix.build>
Annoyed with the interference of the python formatting of generated code (see NixOS#72964), I took matters into my own hands as maintainer of dockerTools. Afterwards, I've created a PR, hoping to unstuck the discussion. @aszlig took notice and thanks to his python ecosystem knowledge, the testing efforts of @blaggacao and @Ma27, and a sense of shared suffering and comraderie we were able to change the situation for the better in NixOS#122201. Now, we have a proper linter that actually helps contributors, so it's time to turn it back on again. I'm glad we could make it happen this quickly! Thanks! This reverts commit 4035049.
So far, we have used "black" for formatting the test code, which is rather strict and opinionated and when used inline in Nix expressions it creates all sorts of trouble.
One of the main annoyances is that when using strings coming from Nix expressions (eg. store paths or option definitions from NixOS modules), completely unrelated changes could cause tests to fail, since eg. black wants lines to be broken.
Another downside of enforcing a certain kind of formatting is that it makes the Nix expression code inconsistent because we're mixing two spaces of indentation (common in nixpkgs) with four spaces of indentation as defined in PEP-8. While this is perfectly fine for standalone Python files, it really looks ugly and inconsistent IMO when used within Nix strings.
What we actually want though is a linter that catches problems early on before actually running the test, because this is actually helping in development because running the actual VM test takes much longer.
This is the reason why I switched from black to pyflakes, because the latter actually has useful checks, eg. usage of undefined variables, invalid format arguments, duplicate arguments, shadowed loop vars and more.
Closes #72964