/snapd

testutil,skills/types,skills,daemon: tweak discovery of know skill types #478

Merged
merged 8 commits into from Feb 25, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@zyga @mvo5 @fgimenez @niemeyer
@zyga
Contributor

zyga commented Feb 12, 2016

This branch moves the discovery of known skill types from skills/ to skills/types.

zyga added some commits Feb 12, 2016

@zyga
testutil: handle interface types in Contains 
This patch fixes Contains (and DeepContains) to handle collections of
interface types. In the past, the code would reject such checks as it
would look at the actual underlying type. Now, if the collection element
is an interface the test checks if the element being looked for
implements that interface.

The change is accompanied with a few simple tests to ensure both failing
and successful cases are working correctly.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
08a068f
@zyga
skills/types: add a way to find all the types 
This patch adds a replacement to skills.LoadBuiltInTypes() that lives in
the skills/types package. The said package is going to contain all the
implemented skill types so it makes sense for the knowledge about
available types to be stored there to avoid circular imports back to the
skills/ package.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
2ccc26b
@zyga
daemon: use skills/types.AllTypes() to add skill types 
This patch changes the mechanism for discovering known types to
AllTypes() since LoadBuiltInTypes() is about to go away.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
a5c2437
@zyga
skills: remove LoadBuildInTypes() 
LoadBuiltInTypes() is now entirely replaced by skills/types/AllTypes().

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

All checks have passed

2 successful checks
@snappy-m-o
Integration tests — Success 60 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
222000d
@zyga
testutils: actually use the dummy tree type 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

All checks have passed

3 successful checks
@snappy-m-o
Integration tests — Success 60 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.5%) to 69.68%
Details
1489cb2
@zyga
Merge branch 'master' of github.com:ubuntu-core/snappy into skill-typ… 
…e-registry

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
Integration tests — 19 tests run, 0 skipped, 7 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.03%) to 70.159%
Details
00cc386
@zyga
Merge branch 'master' of github.com:ubuntu-core/snappy into skill-typ… 
…e-registry

All checks have passed

3 successful checks
@snappy-m-o
Integration tests — Success 64 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — First build on master at 70.306%
Details
52f6ec4
@zyga
Contributor

zyga commented Feb 23, 2016

Merged with master to re-test with integration test fixes
@mvo5
Collaborator

mvo5 commented Feb 23, 2016

There are two test failures in the integration tests that are worth investigating
@fgimenez
Contributor

fgimenez commented Feb 23, 2016

@mvo5 we have had this problem before [1] it appears eventually when trying to install a snap from the local filesystem, maybe related to the snap.yaml filled with '\0'

[1] http://pastebin.ubuntu.com/15126149/
@mvo5
Collaborator

mvo5 commented Feb 23, 2016

I rerun the tests and its happy again
testutil/checkers.go
- containerV.Type().Elem(), elemV.Type())
- return true
+ containerElemType := containerV.Type().Elem()
+ if containerV.Type().Elem().Kind() == reflect.Interface {
@niemeyer

niemeyer Feb 23, 2016

Contributor

This can be containerElemType as well, created right above.

@zyga

zyga Feb 24, 2016

Contributor

Right, nice, I didn't notice that, updated.

@niemeyer
Contributor

niemeyer commented Feb 23, 2016

Nice change, LGTM.
@zyga
testutil: reuse containerElemType 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

All checks have passed

3 successful checks
@snappy-m-o
Integration tests — Success 64 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — First build on master at 70.299%
Details
b25bd0d
daemon/daemon.go
- err = skills.LoadBuiltInTypes(skillRepo)
- if err != nil {
- panic(err.Error())
+ for _, skillType := range types.AllTypes() {
@mvo5

mvo5 Feb 25, 2016

Collaborator

Nice!

skills/types/all_test.go
+
+func (s *AllSuite) TestAllTypes(c *C) {
+ allTypes := types.AllTypes()
+ c.Check(allTypes, Contains, &types.BoolFileType{})
@mvo5

mvo5 Feb 25, 2016

Collaborator

Sorry for being "mr-smarty-pants" here: c.Check(allTypes, DeepEquals, []skills.Type{&types.BoolFileType{}}) would have worked as well. But now Contains knows about interfaces which is nice as well!

testutil/checkers_test.go
@@ -111,12 +146,16 @@ func (s *CheckersS) TestContainsSlice(c *C) {
c.Assert([]int{1, 2, 3}, Contains, 2)
c.Assert([]int{1, 2, 3}, Contains, 3)
c.Assert([]int{1, 2, 3}, Not(Contains), 4)
+ c.Assert([]animal{&dog{}, &cat{}}, Contains, &dog{})
@mvo5

mvo5 Feb 25, 2016

Collaborator

Nice!

@mvo5
Collaborator

mvo5 commented Feb 25, 2016

Looks good!

mvo5 added a commit that referenced this pull request Feb 25, 2016

@mvo5
Merge pull request #478 from zyga/skill-type-registry 
testutil,skills/types,skills,daemon: tweak discovery of know skill types
d2ac5ee

@mvo5 mvo5 merged commit d2ac5ee into snapcore:master Feb 25, 2016
3 checks passed

3 checks passed

Integration tests Success 64 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 70.299%
Details

@zyga zyga deleted the zyga:skill-type-registry branch Mar 8, 2016

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