/snapcraft

Reenable tour command #559

Merged
merged 7 commits into from Jun 9, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@didrocks @fgimenez @elopio @sergiusens
@didrocks
Contributor

didrocks commented Jun 9, 2016

Reenable "snapcraft tour" command with tests.

Added a small logic enhancements backed up with tests to ensure we don't nest default directory inside each other for multiple commands run.

didrocks added some commits Jun 9, 2016

@didrocks
Reenable "tour" command 
Now that we will have the finale "tour" example, reenable the tour command
and related tests.
69fd25e
@didrocks
Use Mark's syntax for output
efe7fe8
@didrocks
Avoid creating snapcraft-tour/snapcraft-… in loop 
When launching multiple time the command, as we could have a destination
path where we either dump the code or create a snapcraft-tour subdirectory
if it exists, multiple snapcraft tour command would create a snapcraft-tour
folded hierarchy.

To avoid that, if we didn't specify any parameter, only try to create a
snapcraft-tour subdir and don't nest under it. Added a test for that.

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.08%) to 95.754%
Details
88059fa
@didrocks
Prettify the path print now that default is ./snapcraft-tour/ 
Better output for the user when the path is joined to a prefix
(printing /tmp/foo/snapcraft-tour instead of /tmp/foo/./snapcraft-tour/).
Default without argument is still, as requested, ./snapcraft-tour/

Some checks were not successful

1 failing and 3 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.08%) to 95.754%
Details
f4429fd
@didrocks
Contributor

didrocks commented Jun 9, 2016

retest this please
@fgimenez
Contributor

fgimenez commented Jun 9, 2016

retest this please
@fgimenez
Contributor

fgimenez commented Jun 9, 2016

retest this please
snapcraft/tests/test_tour.py
def tearDown(self):
with suppress(OSError):
if self.temp_dir:
shutil.rmtree(self.temp_dir)
+ os.chdir(self.cwd)
@elopio

elopio Jun 9, 2016

Member

I missed this in the previous review.
We have a fixture for tmp cwd, which also works as a tempdir. Maybe you can replace your set up and tear down with that fixture, or inherit from the base test case.

@didrocks

didrocks Jun 9, 2016

Contributor

Oh, I didn't see that base class add that fixture, indeed, using this and preventing creating other temp directory when that makes sense for the tests.

@elopio
Member

elopio commented Jun 9, 2016

all the commands need at least one integration test. That's not a blocker for this branch, but a blocker for releasing it. As always, I'm available to help.

didrocks added some commits Jun 9, 2016

@didrocks
Use base class cwd function 
Only create temporary dir and file when needed. Otherwise, use temporary
cwd.

Some checks were not successful

3 failing and 1 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.1%) to 95.799%
Details
1d4bd4b
@didrocks
Add integration tests for the tour command 
Support basics test case for the command itself. Don't repeat all edge cases
covered in unit tests already.

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
50ab07a
@didrocks
Remove useless imports now

All checks have passed

4 successful checks
@snappy-m-o
Examples tests — Success
Details
@snappy-m-o
autopkgtest — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.1%) to 95.798%
Details
46bbfce
@didrocks
Contributor

didrocks commented Jun 9, 2016

pushed the base class changes + integration tests for the tour command.

As told on IRC, I didn't find all other commands beeing tested there, so I took the libery to create a new file for this command only and support 3 main tests cases. All other edge cases (copying to a directory which already has content, copying over a file…) are covered in unit tests.
integration_tests/test_tour.py
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+import integration_tests
+import os
@elopio

elopio Jun 9, 2016

Member

first import os, then an empty line, then integration_tests

@elopio
Member

elopio commented Jun 9, 2016

💯 Full unit coverage, and integration tests for the main cases, thank you!
@sergiusens
Collaborator

sergiusens commented Jun 9, 2016

retest this please

@sergiusens sergiusens merged commit cadeb87 into snapcore:master Jun 9, 2016
4 checks passed

4 checks passed

Examples tests Success
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 95.798%
Details

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

@didrocks @sergiusens
Reenable tour command (#559) 
LP: #1590906
07fb1e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment