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.

Sign up for GitHub

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

Jump to bottom

python3Packages.[psautohint, afdko]: disable slow & failing tests #106464

Merged
merged 2 commits into from Dec 11, 2020
Merged

python3Packages.[psautohint, afdko]: disable slow & failing tests #106464

merged 2 commits into from Dec 11, 2020

Conversation

drewrisinger
Copy link
Contributor

@drewrisinger drewrisinger commented Dec 9, 2020 •
edited

Motivation for this change

When building a NixOS build on Raspberry Pi, found that python3Packages.psautohint was taking FAR too long to build (~5 mins for native x86 build). This disables the slowest tests and fixes up a few minor issues.

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.

@drewrisinger drewrisinger marked this pull request as draft December 9, 2020 17:14
SuperSandro2000
SuperSandro2000 requested changes Dec 9, 2020
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment •
edited

Choose a reason for hiding this comment

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

Can you please squash the commits together to one per package?

@drewrisinger drewrisinger changed the title python3Packages.psautohint: disable slow tests python3Packages.[psautohint, afdko]: disable slow tests Dec 9, 2020
@drewrisinger drewrisinger marked this pull request as ready for review December 9, 2020 20:00
sternenseemann
sternenseemann reviewed Dec 10, 2020
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Thanks for identifying the flaky test! I was meaning to do that.

I'm personally not too concerned about build times, but I don't think there's a reason why we absolutely have to enable all tests.

How come you had to build psautohint btw? I was under the impression psautohint would only need to be built if you'd have to build noto-fonts-emoji or twitter-color-emoji from scratch…

@sternenseemann sternenseemann mentioned this pull request Dec 10, 2020
Merged
10 tasks
@sternenseemann
Copy link
Member

Result of nixpkgs-review pr 106464 run on x86_64-linux 1

11 packages built:
  • noto-fonts-emoji
  • python37Packages.afdko
  • python37Packages.nototools
  • python37Packages.psautohint
  • python38Packages.afdko
  • python38Packages.nototools
  • python38Packages.psautohint
  • python39Packages.afdko
  • python39Packages.nototools
  • python39Packages.psautohint
  • twitter-color-emoji

@drewrisinger
Copy link
Contributor Author

@sternenseemann I'm working on bringing up a Raspberry Pi 4 on NixOS, and the default fonts include noto-color-emoji, and pythonPackages.psautohint somehow needs recompiled for the Raspberry Pi kernel (since that hasn't been transitioned yet to mainline Linux 🤷). I didn't work out the exact dependencies. After this PR, I actually just ended up disabling noto-fonts-emoji b/c that took a VERY long time to build when cross-compiling.

@sternenseemann sternenseemann mentioned this pull request Dec 10, 2020
Closed
@sternenseemann
Copy link
Member

Makes sense! The build process of noto-fonts-emoji (and twitter-color-emoji as a matter of fact) are extremely slow indeed.

Concerning the test suites: As said I don't really have an opinion on whether we should disable tests to reduce build time. As I don't have push access I'm automatically relieved of this decision at least :)

We should however definitely disable test_hashmap_old_version in psautohint.

@drewrisinger drewrisinger changed the title python3Packages.[psautohint, afdko]: disable slow tests python3Packages.[psautohint, afdko]: disable slow & failing tests Dec 10, 2020
@drewrisinger
Copy link
Contributor Author

@sternenseemann It'd probably help those w/ push access to merge this PR if you leave an explicit "approval" review, including your qualms about disabling some tests so they have the full context.

sternenseemann
sternenseemann approved these changes Dec 10, 2020
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

We should however definitely disable test_hashmap_old_version in psautohint until the issue is resolved upstream.

I'm indifferent about disabling tests for build speed reasons.

@sternenseemann sternenseemann mentioned this pull request Dec 10, 2020
Closed
10 tasks
@sternenseemann
Copy link
Member

sternenseemann commented Dec 10, 2020 •
edited

Upstream suggests that test_hashmap_old_version fails due to a change in pytest 6. Due to pytestCheckHook however you can't switch to pytest 5 if I understand that hook correctly. An alternative solution to this issue would be #106609.

@drewrisinger
Copy link
Contributor Author

drewrisinger commented Dec 10, 2020 •
edited

@sternenseemann Alternatively, you CAN override pytestCheckHook:

{
  checkInputs = [ (pytestCheckHook.override{ pytest = pytest_5; }) pytestcov pytest_xdist ];
}

Builds cleanly for me. Can confirm that it passes with the "test_hashmap_old_version" test enabled.

@ofborg ofborg bot requested a review from sternenseemann December 10, 2020 22:55
@drewrisinger
python3Packages.psautohint: disable slow tests
c2c9fcc 
* Reduces test time from ~5 mins to ~30 seconds.
* Also converts to pytestCheckHook for better test control.
* Makes fetchFromGitHub variables match convention.
* remove pytest-randomly.
    pytest-randomly will randomize test order &
    reset random seeds between tests.
    We don't want the random test ordering for reproducible builds, so we
    remove it.
* Fixes flaky test "test_hashmap_old_version" by pinning pytest to v5,
    corresponding to upstream issue:
    https://github.com/adobe-type-tools/psautohint/issues/284#issuecomment-742800965

py3.psautohint: use pytest5
@drewrisinger
python3Packages.afdko: use pytestCheckHook
d6187d5 
Convert to standard pytestCheckHook.
This allows easier testing of derivation build & disabling tests.
Also disable slow tests.
@ofborg ofborg bot requested a review from sternenseemann December 10, 2020 23:06
@drewrisinger
Copy link
Contributor Author

drewrisinger commented Dec 11, 2020 •
edited

Fixes #106558

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 106464 run on x86_64-linux 1

11 packages built:
  • noto-fonts-emoji
  • python37Packages.afdko
  • python37Packages.nototools
  • python37Packages.psautohint
  • python38Packages.afdko
  • python38Packages.nototools
  • python38Packages.psautohint
  • python39Packages.afdko
  • python39Packages.nototools
  • python39Packages.psautohint
  • twitter-color-emoji

@SuperSandro2000 SuperSandro2000 merged commit 9a39c1b into NixOS:master Dec 11, 2020
@drewrisinger drewrisinger deleted the dr-pr-fast-psautohint-tests branch December 11, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@drewrisinger @sternenseemann @SuperSandro2000