/snapcraft

added test for git sources in the get() method in snapcraft/sources.py #217

Closed
wants to merge 5 commits into
from

Conversation

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

fazerlicourice7 commented Jan 10, 2016

No description provided.

@elopio
Member

elopio commented Jan 11, 2016

Hello, thanks for your contribution.
Have you signed the contributors agreement? Please take a look at https://github.com/ubuntu-core/snapcraft/blob/master/CONTRIBUTING.md
@fazerlicourice7
Contributor

fazerlicourice7 commented Jan 11, 2016

Who should I add as project manager?
snapcraft/tests/test_sources.py
+ for source in sources:
+ with self.subTest(key=source):
+ self.assertEqual(
+ snapcraft.sources._get_source_type_from_uri(source), 'git')
@elopio

elopio Jan 11, 2016

Member

In general we should try to avoid testing internal methods directly, and test that code through the exposed API. In this case, instead of testing _get_source_type_from_uri, it would be better to test through the get method. You'll have to patch the subprocess.check_call method so Git.pull doesn't contact the URL, and then just check the arguments passed to that check_call. Here are the details about patching: https://docs.python.org/3/library/unittest.mock.html

I like the scenarios and the subTest.

@elopio
Member

elopio commented Jan 11, 2016

The project manager is Oliver Ries.
@fazerlicourice7
Contributor

fazerlicourice7 commented Jan 11, 2016

Done. I made the change, pushed, squashed the commits, then force pushed.
snapcraft/tests/test_sources.py
+ builddir = 'testbuilddir',
+ options = options)
+
+ mock_pull.assert_called_with()
@elopio

elopio Jan 11, 2016

Member

assert_called_once_with()

@kyrofa
Member

kyrofa commented Jan 11, 2016

@fazerlicourice7 you're missing the bug bits mentioned in the contribution guide @elopio linked to above.
snapcraft/tests/test_sources.py
+ self.source_type = source_type
+ self.source_branch = source_branch
+ self.source_tag = source_tag
+ self.source_subdir = source_subdir
@elopio

elopio Jan 11, 2016

Member

Instead of duplicating the code, it would be nice if you move the one from test_base_plugin to snapcraft/tests.py and use it like tests.MockOptions.

@fazerlicourice7

fazerlicourice7 Jan 11, 2016

Contributor

Where do you want me to move it? snapcraft/tests.py doesn't seem to exist.

snapcraft/tests/test_sources.py
+ for source in sources:
+ with self.subTest(key=source):
+
+ options = MockOptions(source=source,source_type='git')
@elopio

elopio Jan 11, 2016

Member

Here you will have a pep8 error because you are missing a space after the comma.

snapcraft/tests/test_sources.py
+ with self.subTest(key=source):
+
+ options = MockOptions(source=source,source_type='git')
+ local = snapcraft.sources
@elopio

elopio Jan 11, 2016

Member

I don't understand why do you need this alias. I would prefer to just call snapcraft.sources.get.

snapcraft/tests/test_sources.py
+ local = snapcraft.sources
+ local.get(
+ sourcedir = 'testsourcedir',
+ builddir = 'testbuilddir',
@elopio

elopio Jan 11, 2016

Member

since now the sourcedir and builddir are not used in the test, a good practices is to make it explicit that they are dummies (http://xunitpatterns.com/Dummy%20Object.html)
So, please change them like this:
sourcedir = 'dummy',
builddir = 'dummy'

@kyrofa
Member

kyrofa commented Jan 11, 2016

Verified that CLA has been signed.
@elopio
Member

elopio commented Jan 11, 2016

@fazerlicourice7 awesome! Final change request from my side:
update the copyright on all the files you touched to add this year:

Copyright (C) 2015, 2016 Canonical Ltd

Thanks a lot!

@fazerlicourice7 fazerlicourice7 changed the title from added test for git sources in the _get_source_type_from_uri() method to added test for git sources in the get() method in snapcraft/sources.py Jan 11, 2016

@fazerlicourice7
Contributor

fazerlicourice7 commented Jan 11, 2016

Why are these checks failing?
@sergiusens
Collaborator

sergiusens commented Jan 11, 2016

Click on Details for continuous-integration/travis-ci/pr and it will show, here's the inner most link to see the failure https://travis-ci.org/ubuntu-core/snapcraft/jobs/101698500 which mentions:

snapcraft/tests/__init__.py:26:1: E302 expected 2 blank lines, found 1

snapcraft/tests/__init__.py:36:1: E302 expected 2 blank lines, found 1

snapcraft/tests/test_base_plugin.py:28:1: E302 expected 2 blank lines, found 1

snapcraft/tests/test_base_plugin.py:108:31: E128 continuation line under-indented for visual indent

snapcraft/tests/test_base_plugin.py:140:80: E501 line too long (84 > 79 characters)

snapcraft/tests/test_base_plugin.py:141:31: E128 continuation line under-indented for visual indent

snapcraft/tests/test_sources.py:313:1: E302 expected 2 blank lines, found 1

snapcraft/tests/test_sources.py:331:58: W291 trailing whitespace

snapcraft/tests/test_sources.py:338:58: E231 missing whitespace after ','

snapcraft/tests/test_sources.py:340:30: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:340:32: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:340:41: W291 trailing whitespace

snapcraft/tests/test_sources.py:341:29: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:341:31: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:341:40: W291 trailing whitespace

snapcraft/tests/test_sources.py:342:28: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:342:30: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/__init__.py:26:1: E302 expected 2 blank lines, found 1

snapcraft/tests/__init__.py:36:1: E302 expected 2 blank lines, found 1

snapcraft/tests/test_base_plugin.py:28:1: E302 expected 2 blank lines, found 1

snapcraft/tests/test_base_plugin.py:108:31: E128 continuation line under-indented for visual indent

snapcraft/tests/test_base_plugin.py:140:80: E501 line too long (84 > 79 characters)

snapcraft/tests/test_base_plugin.py:141:31: E128 continuation line under-indented for visual indent

snapcraft/tests/test_sources.py:313:1: E302 expected 2 blank lines, found 1

snapcraft/tests/test_sources.py:331:58: W291 trailing whitespace

snapcraft/tests/test_sources.py:338:58: E231 missing whitespace after ','

snapcraft/tests/test_sources.py:340:30: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:340:32: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:340:41: W291 trailing whitespace

snapcraft/tests/test_sources.py:341:29: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:341:31: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:341:40: W291 trailing whitespace

snapcraft/tests/test_sources.py:342:28: E251 unexpected spaces around keyword / parameter equals

snapcraft/tests/test_sources.py:342:30: E251 unexpected spaces around keyword / parameter equals
@fazerlicourice7
Contributor

fazerlicourice7 commented Jan 12, 2016

How can I remove the previous two commits? My earlier one and the one from sergiusens? Every time I try something, something gets messed up.
@kyrofa
Member

kyrofa commented Jan 12, 2016

@fazerlicourice7 assuming you're happy with where your PR is now, you can (and should, if you're following our contribution guide) squash them all down into one well-formatted commit. To do that, get on your branch and run git rebase -i master. The -i flag means "interactive," and it'll cause an editor window to pop up (as if you were making a new commit). The contents of that window will look something like this:

pick 1a2b3c added test for git sources in the _get_source_type_from_uri() method
pick a3b4c5 snap.yaml implementation with support for readme.md and package.yaml 
pick 6a7b8c created test for git source in the get() method in snapcraft/sources.py

# Rebase 710f0f8..a5f4a0d onto 710f0f8
#
# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

Read the comments at the bottom of the editor-- they're helpful. What you want to do is take these three commits and "squash" them into one. So let's squash the last two into the first-- edit it to look like this:

pick 1a2b3c added test for git sources in the _get_source_type_from_uri() method
squash a3b4c5 snap.yaml implementation with support for readme.md and package.yaml 
squash 6a7b8c created test for git source in the get() method in snapcraft/sources.py

# Rebase 710f0f8..a5f4a0d onto 710f0f8
#
# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

Now save and exit the editor, the rebase will run, and it'll pop up another editor window asking you for a commit message for your new squashed commit. You'll notice that it fills the editor with the commit messages from the other three-- clean those up and make it one cohesive commit message. Then save and exit, and the rebase will finish. If you run git log at this point you'll see that you now have a single commit. Note that you just changed your history-- if you try to push to GitHub it'll complain about the fact that your branches have diverged. Tell GitHub to drop its version and take yours by appending --force to the push.

kyrofa and others added some commits Jan 6, 2016

@kyrofa @fazerlicourice7
Add support for mesa libraries. 
Currently Snapcraft adds only a few standard paths to
LD_LIBRARY_PATH. This commit expands that to include
paths used by mesa packages.

Also add an opencv example to exercise this.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

LP: #1531620
f58bf9f
@kyrofa @fazerlicourice7
Autotools plugin: Set autogen.sh executable. 
LP: #1530995

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
1587e87
@kyrofa @fazerlicourice7
Update CONTRIBUTING.md to require a bug. 
LP: #1532195

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
882f6fb
@kyrofa @fazerlicourice7
Ensure rosdep resolves dependencies using Trusty. 
Without this commit, ROS Indigo packages cannot be built with
Snapcraft installed on versions of Ubuntu later than Trusty.

LP: #1532241

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
0dc43df
@fazerlicourice7
added test for git sources in the get() method

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.02%) to 90.192%
Details
178eb27

@fazerlicourice7 fazerlicourice7 closed this Jan 14, 2016

@fazerlicourice7
Contributor

fazerlicourice7 commented Jan 14, 2016

I give up with the git squashing stuff. I'm just going to do it all over again.

@fazerlicourice7 fazerlicourice7 deleted the fazerlicourice7:create-test-git-source branch Jan 14, 2016

smoser pushed a commit to smoser/snapcraft that referenced this pull request Sep 14, 2016

@hglkrijger
port retry code over from 2.0 agent; add unit test; fixes RDBug 52863… 
…41 (#217)
25a1a91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment