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: add core revert test #3348

Merged
merged 27 commits into from Jun 8, 2017
Merged

tests: add core revert test #3348

merged 27 commits into from Jun 8, 2017

Conversation

fgimenez
Copy link
Contributor

@fgimenez fgimenez commented May 18, 2017

This test exercises a core revert on a nested ubuntu-core system, currently only a preinstalled snap is set (network-manager) and the check after the revert only takes into account network configuration. It can be executed with: SPREAD_CORE_CHANNEL=stable spread -v qemu:ubuntu-16.04-64:tests/nested/core-revert.

This PR also includes a refactor of reusable functions from a previous nested task, the actual test is https://github.com/snapcore/snapd/pull/3348/files#diff-4825fd024a235e7895b109ad37072e68

@codecov-io
Copy link

codecov-io commented May 18, 2017

Codecov Report

Merging #3348 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3348      +/-   ##
==========================================
+ Coverage   77.55%   77.55%   +<.01%     
==========================================
  Files         371      371              
  Lines       25519    25519              
==========================================
+ Hits        19790    19792       +2     
+ Misses       3978     3976       -2     
  Partials     1751     1751
Impacted Files Coverage Δ
cmd/snap/cmd_aliases.go 96% <0%> (+2%) ⬆️
interfaces/sorting.go 93.33% <0%> (+3.33%) ⬆️

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 a020a0f...78802c4. Read the comment docs.

@fgimenez
Copy link
Contributor Author

The test case finally reproduces the seccomp issue, with the current status and executed setting SPREAD_CORE_CHANEL=stable it gets stuck here https://github.com/snapcore/snapd/pull/3348/files#diff-4825fd024a235e7895b109ad37072e68R41 (after reverting to stable network-manager doesn't come up and the testbed is not accessible through ssh).

@niemeyer if you could take a look that would be great

Copy link
Collaborator

@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.

Looks good, one (most unrelated for the core of this PR) comment about trap

# create ubuntu-core image
mkdir -p /tmp/work-dir
snap install --devmode --beta ubuntu-image
trap 'snap remove ubuntu-image' EXIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trap will replace other existing traps on the same signal. So if we alread use a trap ... EXIT anwhere this is overriden now. We may consider switching to a helper (add_trap) that stacks the cleanups if we already use traps. Probably material for a new PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mvo5 ! Indeed, for the time being, and given that all the tasks in the nested suite are going to use ubuntu-image, I've moved its install/unistall at the suite level, and btw also removed a trap used for apt, please take another look.

Copy link
Contributor

@morphis morphis left a comment

Choose a reason for hiding this comment

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

LGTM

@mvo5 mvo5 merged commit 8742e4c into snapcore:master Jun 8, 2017
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

A review that I apparently never submitted

QEMU="$(which qemu-system-x86_64)"
;;
i386)
QEMU="$(which qemu-system-i386)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

QEMU can expand to "". This is not a big deal but I'd rather get a qemu-system-x86_64: command not found than a more confusing message below when the first argument of qemu becomes the command. Could we please drop the which?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do in a followup

esac

# create ubuntu-core image
mkdir -p /tmp/work-dir
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer if this was a temporary directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, noted for the followup

@@ -0,0 +1,28 @@
summary: core revert test

systems: [ubuntu-16.04-64]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this test happen on other distributions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, in fact the interesting part happens on the nested machine, if we can install qemu all should be good. I'll try locally with debian/fedora anyway.

@@ -0,0 +1,28 @@
summary: core revert test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please document the intent and mechanics of this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll add a details section

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