osutil: honor SNAPD_UNSAFE_IO for testing #3725

Merged
merged 4 commits into from Aug 21, 2017

Conversation

Projects
None yet
4 participants
Contributor

zyga commented Aug 14, 2017

When SNAPD_UNSAFE_IO is set and we are testing (the executable ends with
.test) then skip some sync calls that are very very heavy. Doing this
can easily cut the timy by an order of magnitude.

As a non-scientific but useful measurement, overlord tests go down from
2 minutes 41 seconds to just 15 seconds. This is not representative to
all modules but when it matters, it is noticeable.

zyga@faroe:~/snapd/overlord> time SNAPD_UNSAFE_IO=0 go test
OK: 41 passed
PASS
ok  	github.com/snapcore/snapd/overlord	156.342s

real	2m41,765s
user	0m12,964s
sys	0m7,061s

zyga@faroe:~/snapd/overlord> time SNAPD_UNSAFE_IO=1 go test
OK: 41 passed
PASS
ok  	github.com/snapcore/snapd/overlord	9.963s

real	0m15,380s
user	0m12,217s
sys	0m1,626s

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

osutil: honor SNAPD_UNSAFE_IO for testing
When SNAPD_UNSAFE_IO is set and we are testing (the executable ends with
.test) then skip some sync calls that are very very heavy. Doing this
can easily cut the timy by an order of magnitude.

As a non-scientific but useful measurement, overlord tests go down from
2 minutes 41 seconds to just 15 seconds. This is not representative to
all modules but when it matters, it is noticeable.

zyga@faroe:~/snapd/overlord> time SNAPD_UNSAFE_IO=0 go test
OK: 41 passed
PASS
ok  	github.com/snapcore/snapd/overlord	156.342s

real	2m41,765s
user	0m12,964s
sys	0m7,061s

zyga@faroe:~/snapd/overlord> time SNAPD_UNSAFE_IO=1 go test
OK: 41 passed
PASS
ok  	github.com/snapcore/snapd/overlord	9.963s

real	0m15,380s
user	0m12,217s
sys	0m1,626s

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

codecov-io commented Aug 14, 2017

Codecov Report

Merging #3725 into master will increase coverage by 0.13%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3725      +/-   ##
==========================================
+ Coverage   75.84%   75.98%   +0.13%     
==========================================
  Files         399      399              
  Lines       34525    34617      +92     
==========================================
+ Hits        26184    26302     +118     
+ Misses       6478     6460      -18     
+ Partials     1863     1855       -8
Impacted Files Coverage Δ
osutil/io.go 69.23% <50%> (+0.48%) ⬆️
overlord/ifacestate/helpers.go 63% <0%> (+0.66%) ⬆️
interfaces/sorting.go 100% <0%> (+1.28%) ⬆️
cmd/snap/cmd_aliases.go 95% <0%> (+1.66%) ⬆️
asserts/assertstest/assertstest.go 71.61% <0%> (+2.05%) ⬆️
cmd/snap-repair/runner.go 92.42% <0%> (+9.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6081a60...34459e0. Read the comment docs.

LGTM, but let's please make the variable more clear:

osutil/io.go
+// Allow disabling sync for testing. This brings massive improvements on
+// certain filesystems (like btrfs) and very much noticeable improvements in
+// all unit tests in genreal.
+var shouldReallySync bool = true
@niemeyer

niemeyer Aug 14, 2017

Contributor

Let's please rename this to snapdUnsafeIO so the relationship to the variable is clear (we need to invert the tests too). We can also drop the = true/false part as it defaults to false.

In fact, we can probably inline:

var snapdUsafeIO = len(os.Args) > 0 && strings.HasSuffix(os.Args[0], ".test") && GetenvBool("SNAPD_UNSAFE_IO")
@zyga

zyga Aug 16, 2017

Contributor

Done

io: rename shouldReallySync to snapdUnsafeIO
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

mvo5 approved these changes Aug 16, 2017

Looks great! One suggestion/nitpick but feel free to ignore and merge anyway.

osutil/io.go
+// Allow disabling sync for testing. This brings massive improvements on
+// certain filesystems (like btrfs) and very much noticeable improvements in
+// all unit tests in genreal.
+var snapdUnsafeIO bool = len(os.Args) > 0 && strings.HasSuffix(os.Args[0], ".test") && GetenvBool("SNAPD_UNSAFE_IO")
@mvo5

mvo5 Aug 16, 2017

Collaborator

(nitpick) - I would declare this at the top of the file somewhere

@zyga

zyga Aug 17, 2017

Contributor

Done

zyga added some commits Aug 17, 2017

osutil: move variable to top of the file
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>

@zyga zyga merged commit b2d84ca into snapcore:master Aug 21, 2017

5 of 7 checks passed

xenial-amd64 autopkgtest finished (failure)
Details
xenial-i386 autopkgtest finished (failure)
Details
artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@zyga zyga deleted the zyga:tweak/allow-disabling-sync-for-tests branch Aug 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment