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

Jump to bottom

duplicity: enable tests #60956

Merged
merged 2 commits into from Jun 4, 2019
Merged

duplicity: enable tests #60956

merged 2 commits into from Jun 4, 2019

Conversation

strager
Copy link
Contributor

@strager strager commented May 5, 2019 •
edited

Motivation for this change

Prevent regressions when upgrading duplicity (or one of its dependencies) by running duplicity's test suite.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from peti May 5, 2019 01:53
@strager strager marked this pull request as ready for review May 5, 2019 02:06
@c0bw3b
Copy link
Contributor

c0bw3b commented May 5, 2019 •
edited

This is failing for me locally

FAILED (failures=21, errors=127, skipped=4)
Test failed: <unittest.runner.TextTestResult run=418 errors=127 failures=21>

Let's try
@GrahamcOfBorg build duplicity

@strager
Copy link
Contributor Author

strager commented May 5, 2019

Interesting. These failures are on NixOS? I'll try to reproduce the test failures on my machine.

@c0bw3b
Copy link
Contributor

c0bw3b commented May 5, 2019

Yes on NixOS with sandboxing. The logs for ofBorg's builds are pretty much the same, on both amd64 and aarch64

peti
peti requested changes May 5, 2019
View reviewed changes
Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

I am hugely in favor of running the test suite! We just need it to succeed first. :-/

@strager
Copy link
Contributor Author

strager commented May 6, 2019

I can reproduce the errors if I enable sandboxing in NixOS.

@strager
Copy link
Contributor Author

strager commented May 7, 2019

I think I fixed the tests with the sandbox enabled.

@ofborg ofborg bot requested a review from peti May 7, 2019 06:41
@c0bw3b
Copy link
Contributor

c0bw3b commented May 7, 2019

Build with tests completed on my side.
So we can check on Darwin let's try again:
@GrahamcOfBorg build duplicity

@c0bw3b
Copy link
Contributor

c0bw3b commented May 7, 2019

Those tests are taking quite a long time (a lot more than the build itself).
On x64: Ran 418 tests in 796.066s
On aarch64: Ran 418 tests in 672.078s

And unfortunately it still fails on Darwin

@peti
Copy link
Member

peti commented May 19, 2019

Those tests are taking quite a long time (a lot more than the build itself).

That's fine, IMHO. That's time well spent verifying that my preferred backup solution actually works. :-)

And unfortunately it still fails on Darwin

I'd be happy to merge the current state as-is if you'd add doCheck = !stdenv.isDarwin. Then we'd have the test suite enabled on Linux'ish platforms at least.

peti
peti requested changes May 19, 2019
View reviewed changes
Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

If you disable the test suite on Darwin only, i.e. by adding doCheck = !stdenv.isDarwin, then we can merge IMHO!

@edolstra
Copy link
Member

I'm not really in favor of enabling tests that are 1) likely to break; 2); slow; 3) require bunch of patches that make it harder to maintain the package.

@strager
duplicity: enable tests
071167e 
* Install required tool dependencies ('tar', 'rdiff', etc.)
* Run installed duplicity instead of source tree duplicity
* Fix shebang of scripts in testing/overrides/bin/
* Disable some unhelpful developer-only tests
* Disable a test broken on Linux (root cause unknown)
* Work around low file descriptor soft limit for Darwin builders
@strager
duplicity: speed up testing
74bb653 
Apply some upstream duplicity patches which optimize installCheckPhase.
On my laptop, this lowers duplicity's total build time from 8 m 42 s to
6 m 50 s (-21%).
@strager
Copy link
Contributor Author

strager commented Jun 3, 2019 •
edited

I reduced test run time by almost two minutes, and I disabled testing on macOS to make CI happy.

@ofborg ofborg bot requested a review from peti June 3, 2019 17:51
@peti peti merged commit 3467918 into NixOS:master Jun 4, 2019
@simon-weber simon-weber mentioned this pull request Jul 7, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peti peti Awaiting requested review from peti

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@strager @c0bw3b @peti @edolstra