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

tests: install most important snaps #3692

Merged

Conversation

sergiocazzolato
Copy link
Collaborator

This branch adds a tests which installs a set of snaps form different channels.

The basic idea is to test that most important snaps can be installed and there is not any failure in the process. This test will be executed nightly with spread cron.

The test autodetects the channel and mode for each snap.

@@ -0,0 +1,82 @@
summary: Check install popular snaps
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not super keen on having this in the main suite where it blocks all PRs.

Ideally (at least for me, obviously) this would be a nightly suite that we use as a go/no-go test for releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is what is happening, note that the test is makred with "manual: true" below. However I think we should mention this in either the description or the summay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha, I missed that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added details explaning that

manual: true

environment:
# High Profile
Copy link
Collaborator

Choose a reason for hiding this comment

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

Who will maintain this list over time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I'll do until we implement a way to get automatically that list. I'll be working on that.

break
done

snap list | grep -q -E $SNAP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use MATCH here

Copy link
Contributor

@mvo5 mvo5 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 this PR! Some nitpicks and suggestions inside but no blockers.

@@ -0,0 +1,82 @@
summary: Check install popular snaps
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is what is happening, note that the test is makred with "manual: true" below. However I think we should mention this in either the description or the summay.

SNAP/vault: vault

execute: |
CHANNELS=(stable candidate beta edge)
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick, not really important as we use bash in spread anyway but I can't help it, years of ensuring maintainer scripts work with dash (instead of bash) have tained me :)) - if you want to write this in portable "sh" instead of bash you could simpliy write it as.

CHANNELS="stable candidate beta edge"
for CHANNEL in $CHANNELS; do
...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, change done, thanks for reviewing

execute: |
CHANNELS=(stable candidate beta edge)
for CHANNEL in "${CHANNELS[@]}"; do
CHANNEL_INFO="$(snap info $SNAP | grep " $CHANNEL: ")" || exit
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick) I find it harder to read the postfix || && than using "if ...; then", I would prefer converting it. I assume this is for snaps that are not actually in the store (anymore) ? If so, maybe we can also have an echo like "skippping test for $SNAP" or something? And if spread grows a warning feaure we can emit a proper warning here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, I'll add the echo.

@codecov-io
Copy link

codecov-io commented Aug 9, 2017

Codecov Report

Merging #3692 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3692      +/-   ##
==========================================
- Coverage   75.81%   75.79%   -0.02%     
==========================================
  Files         402      402              
  Lines       34745    34745              
==========================================
- Hits        26341    26336       -5     
- Misses       6529     6533       +4     
- Partials     1875     1876       +1
Impacted Files Coverage Δ
wrappers/binaries.go 72.72% <0%> (-6.82%) ⬇️
cmd/snap/cmd_aliases.go 93.33% <0%> (-1.67%) ⬇️
interfaces/sorting.go 98.71% <0%> (-1.29%) ⬇️
overlord/ifacestate/helpers.go 62.33% <0%> (ø) ⬆️

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 6cc76e4...bc6e0fb. Read the comment docs.

@sergiocazzolato
Copy link
Collaborator Author

sergiocazzolato commented Aug 17, 2017

@zyga @mvo changes ready, just failing some tests because the refresh-all tests are failing on autopkgtest

@zyga zyga merged commit 789c8b4 into snapcore:master Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants