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: spread test snap dir migration #11280

Merged
merged 5 commits into from
Jan 24, 2022

Conversation

MiguelPires
Copy link
Contributor

Test that the experimental "hidden snap folder" migrates the snap user data and sets the right environment variables after a refresh.

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2022

Codecov Report

Merging #11280 (89460e7) into master (e274227) will decrease coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11280      +/-   ##
==========================================
- Coverage   78.34%   78.24%   -0.11%     
==========================================
  Files         927      927              
  Lines      105735   105979     +244     
==========================================
+ Hits        82838    82923      +85     
- Misses      17745    17899     +154     
- Partials     5152     5157       +5     
Flag Coverage Δ
unittests 78.24% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
overlord/configstate/configcore/netplan.go 9.52% <0.00%> (-11.64%) ⬇️
gadget/update.go 91.30% <0.00%> (-4.21%) ⬇️
overlord/ifacestate/helpers.go 76.96% <0.00%> (-0.49%) ⬇️
gadget/gadget.go 89.90% <0.00%> (ø)
cmd/snap/cmd_debug_state.go 70.64% <0.00%> (+0.45%) ⬆️
overlord/hookstate/hookmgr.go 75.32% <0.00%> (+0.64%) ⬆️

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 e274227...89460e7. Read the comment docs.

rm -rf "$BLOB_DIR"

snap set system experimental.hidden-snap-folder!
snap remove "$SNAP_NAME"
Copy link
Collaborator

Choose a reason for hiding this comment

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

what reminds me the tests/lib/reset.sh needs to be updated to remove /root/.snap and /home/test/.snap

#shellcheck source=tests/lib/store.sh
. "$TESTSLIB"/store.sh

snap install "$SNAP_NAME"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would probably be better to avoid poking the real store and use make_snap_installable to generate the necessary assertions. Or you could replace the store completely and just use local snaps, where the refresh could then be replaced by another installation of the same snap which will bump its revision. IIRC from the perspective of migration this should be identical to a refresh from store.

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 changed the refresh and install for local installs. Thanks for the suggestion

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

I think we should also check the interaction with snapshots, snap save/snap restore.

tests/main/hidden-snap-dir/task.yaml Outdated Show resolved Hide resolved
tests/main/hidden-snap-dir/task.yaml Outdated Show resolved Hide resolved
tests/main/hidden-snap-dir/task.yaml Outdated Show resolved Hide resolved
tests/main/hidden-snap-dir/task.yaml Outdated Show resolved Hide resolved
Test that a snapshot taken before migrating the snap can be
restored after the migration. Also, replace the installs with
'snaps-state install-local' and other minor improvements.
Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Thank you for this PR - looks very nice, thanks for this careful level of testing. Some small nitpicks/suggestions inline that I hope you find helpful but no blockers really.

"$TESTSTOOLS"/snaps-state install-local "$NAME"

restore: |
snap set system experimental.hidden-snap-folder!
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick) snap unset system experimental.hidden-snap-folder is maybe a bit easier to read

not test -d "$HOME"/.snap/data

echo "Take a snapshot"
"$NAME".cmd echo "prev_data" > "$HOME"/snap/"$NAME"/current/data
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick^3) There is a test-snapd-tools.echo" command so this *could* use just that insead of going via cmd echo` but it's really a minor thing, feel free to ignore

tests/main/hidden-snap-dir/task.yaml Show resolved Hide resolved
echo "Restore snapshot and check data was restored"
snap restore "$snapshot"
# shellcheck disable=SC2002
cat "$HOME"/.snap/data/"$NAME"/x2/data | MATCH "prev_data"
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick) MATCH "prev_data" < "$HOME"/.snap/data/"$NAME"/x2/data would also work and avoid the shellcheck disable - or am I missing something? Not really important just a tiny bit more compact because it avoids the extra shellcheck disable line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's good too. I just went with this option because in a previous PR someone preferred the cat and UUoC warning disable since it was more explicit. But this one is very simple, so I think < is quite readable.

@mvo5 mvo5 merged commit 45dd7cc into snapcore:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants