interfaces/mount: make Change.Perform testable and test it #3971

Merged
merged 5 commits into from Oct 5, 2017

Conversation

Projects
None yet
4 participants
Contributor

zyga commented Sep 27, 2017

This patch adds the necessary system call mocking to Change.Perform
and adds a complementary suite of unit tests for all code paths.
This is a preparation step for increasing the complexity of Perform.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

interfaces/mount: make Change.Perform testable and test it
This patch adds the necessary system call mocking to Change.Perform
and adds a complementary suite of unit tests for all code paths.
This is a preparation step for increasing the complexity of Perform.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

codecov-io commented Sep 27, 2017

Codecov Report

Merging #3971 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3971      +/-   ##
==========================================
+ Coverage   75.71%   75.75%   +0.03%     
==========================================
  Files         424      424              
  Lines       36608    36607       -1     
==========================================
+ Hits        27717    27730      +13     
+ Misses       6945     6931      -14     
  Partials     1946     1946
Impacted Files Coverage Δ
cmd/snap-update-ns/change.go 100% <100%> (+18.18%) ⬆️
overlord/ifacestate/helpers.go 63% <0%> (+0.66%) ⬆️

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 8ba90d1...b9cd72e. Read the comment docs.

Very nice! Just very minor nitpicks.

interfaces/mount/change_test.go
-type changeSuite struct{}
+type changeSuite struct {
+ sys *mount.SyscallRecorder
+ restore func()
@stolowski

stolowski Oct 3, 2017

Contributor

Since you suggested similar thing to me today (and I kind like that) ;), let's use BaseTest and get rid of restore before it grows

@zyga

zyga Oct 3, 2017

Contributor

Ack

interfaces/mount/change_test.go
+func (s *changeSuite) TestPerformMount(c *C) {
+ chg := &mount.Change{Action: mount.Mount, Entry: mount.Entry{Name: "source", Dir: "target", Type: "type"}}
+ err := chg.Perform()
+ c.Assert(err, IsNil)
@stolowski

stolowski Oct 3, 2017

Contributor

c.Assert(chg.Perform(), IsNil)

applies to asserts further down too.

@zyga

zyga Oct 3, 2017

Contributor

Ack

zyga added some commits Oct 3, 2017

interfaces/mount: abbreviate test code
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/mount: use BaseTest helper
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

zyga commented Oct 3, 2017

@stolowski both done now

Thanks!

interfaces/mount/change_test.go
+
+// Change.Perform returns errors from unmount system call
+func (s *changeSuite) TestPerformUnountError(c *C) {
+ s.sys.InsertFault(`unmount "target" 8`, errTesting)
@mvo5

mvo5 Oct 4, 2017

Collaborator

Maybe a comment like above: // The flag 8 is UMOUNT_NOFOLLOW ? I also wonder if there is a way to make this easier to read in the tests. Not sure how hard it would be to resolve the flags in the recorder to symbolic names?

@zyga

zyga Oct 4, 2017

Contributor

I'll look into this.

@zyga

zyga Oct 4, 2017

Contributor

Done, this is symbolic now

zyga added some commits Oct 4, 2017

interfaces/mount: use symbolic UMOUNT_NOFOLLOW in tests
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

mvo5 approved these changes Oct 5, 2017

@mvo5 mvo5 merged commit 61d5d61 into snapcore:master Oct 5, 2017

6 of 7 checks passed

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

@zyga zyga deleted the zyga:feature/mount-change-testing branch Oct 5, 2017

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