Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
tests: enable main suite on fedora #3505
Conversation
sergiocazzolato
changed the title from
PLEASE IGNORE: Enable more tests for suse and fedora
to
PLEASE IGNORE: Enabling main test suite for fedora
Jul 10, 2017
|
@morphis @sergiocazzolato I'm taking this one down as it sounds like there's work to do before it can be integrated. Please feel free to reopen or create a new PR once ready. |
niemeyer
closed this
Jul 11, 2017
sergiocazzolato
reopened this
Jul 11, 2017
|
@niemeyer I'll pushed a change to fix all the fedora tests. |
codecov-io
commented
Jul 11, 2017
•
Edited 1 time
-
codecov-io
Jul 11, 2017
-
Jul 11, 2017
Codecov Report
@@ Coverage Diff @@
## master #3505 +/- ##
==========================================
- Coverage 74.98% 74.97% -0.01%
==========================================
Files 382 382
Lines 33116 33116
==========================================
- Hits 24831 24830 -1
- Misses 6485 6486 +1
Partials 1800 1800
Continue to review full report at Codecov.
|
sergiocazzolato
changed the title from
PLEASE IGNORE: Enabling main test suite for fedora
to
Enabling main test suite for fedora
Jul 12, 2017
| @@ -61,7 +61,7 @@ backends: | ||
| kernel: GRUB 2 | ||
| workers: 4 | ||
| - fedora-25-64: | ||
| - workers: 1 | ||
| + workers: 2 |
morphis
Jul 13, 2017
Contributor
Please limit this to 1 again. @niemeyer asked me to use just a single machine for both Fedora and openSUSE.
niemeyer
Jul 14, 2017
Contributor
Yes, let's please keep it down to 1 while we have so many disabled tests. We should only increase it further once this becomes the bottleneck (IOW, it finishes last).
sergiocazzolato
Jul 17, 2017
Contributor
It is running 130 tasks, just 1 worker will delay the whole execution, see the log: https://paste.ubuntu.com/25113080/
niemeyer
changed the title from
Enabling main test suite for fedora
to
tests: enable main suite on fedora
Jul 14, 2017
| @@ -61,7 +61,7 @@ backends: | ||
| kernel: GRUB 2 | ||
| workers: 4 | ||
| - fedora-25-64: | ||
| - workers: 1 | ||
| + workers: 2 |
morphis
Jul 13, 2017
Contributor
Please limit this to 1 again. @niemeyer asked me to use just a single machine for both Fedora and openSUSE.
niemeyer
Jul 14, 2017
Contributor
Yes, let's please keep it down to 1 while we have so many disabled tests. We should only increase it further once this becomes the bottleneck (IOW, it finishes last).
sergiocazzolato
Jul 17, 2017
Contributor
It is running 130 tasks, just 1 worker will delay the whole execution, see the log: https://paste.ubuntu.com/25113080/
| +#!/bin/bash | ||
| + | ||
| +wait_for_file() { | ||
| + the_file="$1" |
niemeyer
Jul 14, 2017
Contributor
Across all shell files and all yaml files in this PR, let's please use 4 spaces of indentation consistently.
This one file has three different indentations in 15 lines, for example.
| @@ -4,19 +4,28 @@ systems: [-ubuntu-core-16-*] | ||
| execute: | | ||
| echo "When some snaps are installed" | ||
| - . $TESTSLIB/snaps.sh | ||
| + . "$TESTSLIB/snaps.sh" |
niemeyer
Jul 14, 2017
Contributor
No need for that I think. It's a good practice in general, but this is our own garden. We'll never have spaces in $TESTSLIB.
chipaca
Jul 24, 2017
Member
right, but as you say it's good practice, and shellcheck will complain about these, so why not do it?
niemeyer
Jul 24, 2017
Contributor
Mainly because it's more noise, increasing both the cost of reading and the cost of reviewing as we have dozens of lines in this PR which don't really change any logic.
| - . $TESTSLIB/dirs.sh | ||
| + # For now we use the Fedora specific snap-mgmt script but as soon | ||
| + # as we have a generic one we can use cross-distro we need to | ||
| + # change this. |
| + --snap-mount-dir=$SNAPMOUNTDIR \ | ||
| + --purge | ||
| + else | ||
| + echo "And snapd is purged" |
| @@ -29,7 +29,7 @@ restore: | | ||
| exit | ||
| fi | ||
| - . $TESTSLIB/store.sh | ||
| + . "$TESTSLIB/store.sh" |
niemeyer
Jul 14, 2017
Contributor
Again, no need for those replacements. This increases the size of the overall diff to be reviewed and touches the git history without adding any value into the actual logic.
| + . "$TESTSLIB/files.sh" | ||
| + . "$TESTSLIB/store.sh" | ||
| + init_fake_refreshes test-snapd-tools "$BLOB_DIR" | ||
| + wait_for_file "$BLOB_DIR"/test-snapd-tools*fake1*.snap 4 .5 |
niemeyer
Jul 14, 2017
Contributor
Similarly, there's no need to quote every single instance of our own variables.
| + mkdir -p "$SNAP_INSTALL_DIR" | ||
| + cp -ra "$TESTSLIB"/snaps/test-snapd-tools/* "$SNAP_INSTALL_DIR" | ||
| + sed -i 's/test-snapd-tools/not-test-snapd-tools/g' "$SNAP_INSTALL_DIR/meta/snap.yaml" | ||
| + snapbuild "$SNAP_INSTALL_DIR" . |
niemeyer
Jul 14, 2017
Contributor
Same! Sorry for bothering, but this is a pretty large diff, and those quotings are a significant part of it, without any real advantages for us.
| echo "When a temporary file is created by one snap" | ||
| - expect -d -f tmp-create.exp | ||
| + expect -d -f tmp-create.exp "$SNAPMOUNTDIR" |
sergiocazzolato
Jul 17, 2017
Contributor
The snap is leaving in $SNAPMOUNTDIR the .sh, and in $SNAP_INSTALL_DIR the commands.
sergiocazzolato
Jul 24, 2017
Contributor
As we discussed in irc, i'll work after this branch to unify the "_" convention for variables
| - /snap/bin/snapctl-from-snap.snapctl-set foo=123 | ||
| - /snap/bin/snapctl-from-snap.snapctl-get foo | MATCH 123 | ||
| + $SNAPMOUNTDIR/bin/snapctl-from-snap.snapctl-set foo=123 | ||
| + $SNAPMOUNTDIR/bin/snapctl-from-snap.snapctl-get foo | MATCH 123 |
niemeyer
Jul 14, 2017
Contributor
... and no quotings here, which is fine. Just raising here to say that a lot of our content isn't quoted, and this isn't a real problem we have today.
and others
added some commits
Jun 8, 2017
Some checks were not successful
Some checks were not successful
All checks have passed
sergiocazzolato
referenced this pull request
Jul 19, 2017
Merged
tests: enable main suite for opensuse #3604
sergiocazzolato
added some commits
Jul 19, 2017
Some checks were not successful
All checks have passed
sergiocazzolato
added some commits
Jul 24, 2017
morphis commentedJun 21, 2017
•
Edited 1 time
-
niemeyer
Jul 24, 2017
No description provided.