1145165 - tag-release.py script needs a refactory

Status: RESOLVED INCOMPLETE

(Release Engineering :: General, defect)

Description

[Massimo Gervasini [:massimo]]

As part of understanding the impact of the pull timeout changes, we need to add tests on the tag-release.py script. Unfortunately, this script has a '-' in its name and so it becomes hard to import and mock its functions.

Probably the best approach here is to move the functions provided by tag-release.py to a separate library.

Comment 1

[Justin Wood (:Callek)]

fwiw http://stackoverflow.com/questions/8350853/how-to-import-python-module-when-module-name-has-a-dash-or-hyphen-in-it has some solutions that don't involve a complete refactor.

Albeit a refactor/rename would likely be ideal here anyway.

Comment 2

[Massimo Gervasini [:massimo]]

Attachment: [tools] Bug 1145165 - created release.tag library, added tests.patch

Thanks Callek for the link!

changes in this patch:

* moved getBumpCommitMessage, getTagCommitMessage, bump, tag, tagRepo and tagOtherRepo functions from scripts/release/tag-release.py to lib/python/release/tag.py
* added unittests (lib/python/mozilla_buildtools/test/test_release_tag.py)
* coverage is 46% (in tag.py) - everything  is tested except for closures - do we need to refactor these as well to increase the coverage?

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8580269 [details] [diff] [review]
[tools] Bug 1145165 - created release.tag library, added tests.patch

Sorry for the delay reviewing this. The file move seems fine to me, except for some cleanup in tag.py (see below). I'm not sure where you're going with the tests though, they seem a bit incomplete in terms of verifying the functionality. Perhaps I'm missing the point though.

>diff --git a/lib/python/mozilla_buildtools/test/test_release_tag.py b/lib/python/mozilla_buildtools/test/test_release_tag.py
>+class TestReleaseTag(unittest.TestCase):
>+    def test_getBumpCommitMessage(self):
>+    def test_getTagCommitMessage(self):

These two seem fine.

>+    def test_bump(self, m_bumpFile):
...
>+        with mock.patch('__builtin__.open', mock.mock_open(read_data=content)):
>+            self.assertTrue(bump(repo, bumpFiles, versionKey) is None)

Doesn't seem to verify the content is modified correctly, just that bump() doesn't error ?

>+    def test_tag(self, m_run_cmd):

A lot of overlap with test_getTagCommitMessage ?

>+    def test_tagRepo(self, m_aap, m_mercurial, m_retry):
...
>+        tagRepo(config=config, repo=repo, revision=revision, reponame=reponame,
>+                tags=tags, bumpFiles=bumpFiles, relbranch=relbranch,
>+                pushAttempts=pushAttempts)

Relies on tagRepo to raise an exception if something goes wrong ? Similar for test_tagOtherRepo.

>diff --git a/scripts/release/tag-release.py b/lib/python/release/tag.py
>similarity index 55%
>copy from scripts/release/tag-release.py
>copy to lib/python/release/tag.py

The first few lines are:

1 #!/usr/bin/env python
2 
3  import logging
4  from os import path
5  import subprocess
6  import sys
7  
8  sys.path.append(path.join(path.dirname(__file__), "../../lib/python"))
9  logging.basicConfig(
10     stream=sys.stdout, level=logging.INFO, format="%(message)s")
11 log = logging.getLogger(__name__)

* the hashbang (1) can definitely go from a module
* sys.path.append (2) is the responsibility of whatever is using the module
* there's no logging in the module, so 3 and 9-11 can go