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/lib/test*: remove perl test driver #96396

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Aug 26, 2020

This has been deprecated in 20.03, and all tests have been migrated to
the python framework, effectively making this dead code.

Motivation for this change

#72828

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

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 26, 2020
@worldofpeace worldofpeace added this to the 20.09 milestone Aug 27, 2020
@worldofpeace
Copy link
Contributor

I think that's my only requested change. Everything else looks good.

Copy link
Member

@aszlig aszlig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, however could we please address #72964 before merging this? Most of the tests in nixpkgs are rather simple, but as soon as you use Nix to parameterize tests you'll get random failures with black.

@flokli
Copy link
Contributor Author

flokli commented Aug 27, 2020

@aszlig I don't see how #72964 should block merging this in. I agree this would be nice, but I don't know if finding a linter ignoring line lengths is too hard, or if just noone has picked up the task yet. All of nixpkgs managed to somewhat work around these issues too :-)

This perl driver has been deprecated for a whole release now, and removing it from nixpkgs for the 20.09 release has been announced as well.

This has been deprecated in 20.03, and all tests have been migrated to
the python framework, effectively making this dead code.
@khumba
Copy link
Contributor

khumba commented Aug 28, 2020

LGTM, however could we please address #72964 before merging this? Most of the tests in nixpkgs are rather simple, but as soon as you use Nix to parameterize tests you'll get random failures with black.

Or if you disable linting on your test suite wholesale, then an evaluation of all your packages produces a screenful of Linting is disabled! lines. I've created #96515 to hopefully start a discussion.

Before removing the Perl driver, I'm more concerned about #86889. It's pretty easy to hang the Python test driver with a test that produces a lot of output on stderr. In my experience it isn't even intermittently; if the test writes thousands of lines, it's going to hang. This is a regression compared to the Perl driver.

@flokli
Copy link
Contributor Author

flokli commented Aug 28, 2020

Before removing the Perl driver, I'm more concerned about #86889. It's pretty easy to hang the Python test driver with a test that produces a lot of output on stderr. In my experience it isn't even intermittently; if the test writes thousands of lines, it's going to hang. This is a regression compared to the Perl driver.

This has been fixed by #96254. I updated #86889.

@flokli
Copy link
Contributor Author

flokli commented Aug 28, 2020

Or if you disable linting on your test suite wholesale, then an evaluation of all your packages produces a screenful of Linting is disabled! lines. I've created #96515 to hopefully start a discussion.

For all of 20.03, if you still have been using the non-python test driver, there were 3 lines of warnings:

Perl VM tests are deprecated and will be removed for 20.09.
Please update your tests to use the python test driver.
See https://github.com/NixOS/nixpkgs/pull/71684 for details.

I don't see why getting it down to 1 line per test (after migrating to the python test and setting skipLint to true) is so bad we need to introduce another option in #96515.

@worldofpeace worldofpeace merged commit f2d0a68 into NixOS:master Aug 28, 2020
@Ma27
Copy link
Member

Ma27 commented Aug 28, 2020

@aszlig when hacking on tests you probably want to use skipLint = true;, IMHO those linting "errors" become rather annoying otherwise :)

@dasJ
Copy link
Member

dasJ commented Aug 28, 2020

@flokli For my private tests I simply use skipLint = true; but otherwise I'd have fixed that ;)

@flokli flokli deleted the remove-perl-test-driver branch August 28, 2020 17:21
@flokli
Copy link
Contributor Author

flokli commented Aug 28, 2020

Let's talk about how to make the python linter less annoying in #72964.

Thanks for merging this PR - 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.

@Ma27
Copy link
Member

Ma27 commented Aug 28, 2020

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" ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants