Merge lp:~jameinel/bzr/2.5-verbosity-knob-812928 into lp:bzr

[John A Meinel]

Bug #812928, allow configuring how verbose the package freshness checks are.

This adds a verbosity knob for the new "Is this packaging branch up-to-date", and refactors the code a bit to make it more testable. The config entry is
  bzr.plugins.launchpad.packaging_verbosity

The possible values are:

off
  Displays nothing, and skips all checks.

minimal
  Only displays single-line messages when out-of-date, but never for
  missing or up-to-date

  1.0-1ubuntu1 is OUT-OF-DATE, Ubuntu Natty has 1.0-1ubuntu2

short
  Displays single-line messages for out-of-date, missing, and up-to-date

  1.0-1ubuntu1 is OUT-OF-DATE, Ubuntu Natty has 1.0-1ubuntu2

  Ubuntu Natty is MISSING a version

  1.0-1ubuntu2 is CURRENT in Ubuntu Natty

all (default)
  Displays multi-line messages for all 3 states

  Most recent Ubuntu Natty version: 1.0-1ubuntu2
  Packaging branch version: 1.0-1ubuntu1
  Packaging branch status: OUT-OF-DATE

  Most recent Ubuntu Natty version: MISSING

  Most recent Ubuntu Natty version: 1.0-1ubuntu2
  Packaging branch status: CURRENT

[Vincent Ladeuil (vila)]

On behalf of UDD users: Thank You !

27 + c = the_branch.get_config()
28 + verbosity = c.get_user_option('bzr.plugins.launchpad.packaging_verbosity')

You're a bit out of fashion here ;)

You can start using the the stck-based config here.

This can take various forms:

- the consensus about the option names would be more
   'launchpad.packaging_verbosity' which is still long and may be
   slightly unclear, how about 'lp.package_freshness' ?

- use the stack-based implementation (even if you target 2.4)

    c = config.BranchStack(the_branch)
    verbosity = c.get('bzr.plugins.launchpad.packaging_verbosity')

  Declare your option:

     option_registry.register('xxx, Option('xxx', default='off'),
                              help='xxx is so nice, use it.')

113 +def report_freshness(the_branch, verbosity, latest_pub):

This function seem to mix 3 different things:
- get the LatestPublication data
- time them
- report to the user

I'm not sure we need to time them (mutter does that already no ?)

I think the code will be clearer and easier to test if this was
split.

234 +class TestReportFreshness(tests.TestCaseWithMemoryTransport):

This can then be split/parametrized so the display tests just
don't need a branch at all.

252 + orig_log_len = len(self.get_log())

In many discussions about self.get_log() we mentioned that only a
few tests were requiring get_log() and I seem to remember that
everybody agreed on that.

Here, you have to filter almost everything, istm that split the
display out will make the filtering uneeded.

That will also probably reduce the duplication like:

270 + b = self.make_trivial_branch()

which strongly want to be in a setUp() ;)

Now, how should I vote...

The above are not enough to block.

On the other hand, I would be *highly* surprised if nobody
bikeshed there or ask for some different details, so istm we
*will* modify these tests.

So BB:tweak as much as you can ;)

review: Needs Fixing

[John A Meinel (jameinel)]

sent to pqm by email

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 7/21/2011 3:06 PM, Vincent Ladeuil wrote:
> Review: Needs Fixing
> On behalf of UDD users: Thank You !
>
> 27 + c = the_branch.get_config()
> 28 + verbosity = c.get_user_option('bzr.plugins.launchpad.packaging_verbosity')
>
> You're a bit out of fashion here ;)
>
> You can start using the the stck-based config here.
>
> This can take various forms:
>
> - the consensus about the option names would be more
> 'launchpad.packaging_verbosity' which is still long and may be
> slightly unclear, how about 'lp.package_freshness' ?

Except, this is about package_branch_freshness, and I don't have good
ideas for what the values should be. "package_branch_freshness_check"
could work, but that is much more verbose.

freshness_verbosity
freshness_check
up_to_date_check

And the _check form doesn't really lend itself to "short" vs "minimal",
just to on/off.

I'm ambivalent about "lp." vs "launchpad.". I like the shortness, but it
does seem best to have unique namespace, and someone might create a
plugin named 'lp'. Then again, launchpad already provides lp...

I'll go with "launchpad.packaging_verbosity" for now. And I'll update
the docs for this.

>
> - use the stack-based implementation (even if you target 2.4)
>
> c = config.BranchStack(the_branch)
> verbosity = c.get('bzr.plugins.launchpad.packaging_verbosity')
>
> Declare your option:
>
> option_registry.register('xxx, Option('xxx', default='off'),
> help='xxx is so nice, use it.')

I'm actually thinking to target 2.3 in the middle-term, because that is
where the developers using package branches are going to be at right
now. We may decide not to do it, but I'd like to at least plan for
backporting this for now. The time it takes to clean up now is the same
as it will take in the future.

>
> 113 +def report_freshness(the_branch, verbosity, latest_pub):
>
> This function seem to mix 3 different things:
> - get the LatestPublication data
> - time them
> - report to the user
>
> I'm not sure we need to time them (mutter does that already no ?)

Mutter reports the time-since-started, and with a small amount of math
you can work out the delta. But, you still need to mutter *something*,
and it is easier to just mutter the info I wanted.

Long term, we can remove them. Short term, I think it is important to be
monitoring how much overhead adding this code is costing us, vs the
benefit to the users.

>
> I think the code will be clearer and easier to test if this was
> split.
>

Ultimately, there will always be a 'do what I want' function, and the
size of the 3 steps is pretty small. I did refactor it, but it didn't
change the number of tests I needed to do for the reporting function. It
made them a little bit smaller, I guess.

> 234 +class TestReportFreshness(tests.TestCaseWithMemoryTransport):
>
> This can then be split/parametrized so the display tests just
> don't need a branch at all.
>
> 252 + orig_log_len = len(self.get_log())
>
> In many discussions about self.get_log() we mentioned that only a
> few tests were requiring get_log() and I seem to remember that
> everybody agreed on ...

=== modified file 'bzrlib/plugins/launchpad/__init__.py'
--- bzrlib/plugins/launchpad/__init__.py	2011-07-18 16:51:56 +0000
+++ bzrlib/plugins/launchpad/__init__.py	2011-07-22 12:04:10 +0000
@@ -40,10 +40,6 @@
 
 # see http://wiki.bazaar.canonical.com/Specs/BranchRegistrationTool
 
-import time
-
-# Since we are a built-in plugin we share the bzrlib version
-
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 from bzrlib import (
@@ -56,6 +52,7 @@
     branch as _mod_branch,
     bzrdir,
     lazy_regex,
+    # Since we are a built-in plugin we share the bzrlib version
     version_info,
     )
 from bzrlib.commands import (
@@ -502,39 +499,19 @@
     info = _get_package_branch_info(the_branch.base)
     if info is None:
         return
+    c = the_branch.get_config()
+    verbosity = c.get_user_option('launchpad.packaging_verbosity')
+    if verbosity is not None:
+        verbosity = verbosity.lower()
+    if verbosity == 'off':
+        trace.mutter('not checking %s because verbosity is turned off'
+                     % (the_branch.base,))
+        return
     archive, series, project = info
     from bzrlib.plugins.launchpad import lp_api_lite
-    t = time.time()
     latest_pub = lp_api_lite.LatestPublication(archive, series, project)
-    latest_ver = latest_pub.get_latest_version()
-    t_latest_ver = time.time() - t
-    trace.mutter('LatestPublication.get_latest_version took %.3fs'
-                 % (t_latest_ver,))
-    if latest_ver is None:
-        trace.note('Could not find a published version for packaging branch:\n'
-                   '  %s' % (the_branch.base,))
-        return
-    t = time.time()
-    tags = the_branch.tags.get_tag_dict()
-    t_tag_dict = time.time() - t
-    trace.mutter('LatestPublication get_tag_dict took: %.3fs' % (t_tag_dict,))
-    if latest_ver in tags:
-        trace.note('Found most recent published version: %s'
-                   ' in packaging branch:\n  %s'
-                   % (latest_ver, the_branch.base))
-    else:
-        place = archive.title()
-        if series is not None:
-            place = '%s/%s' % (place, series.title())
-        best_tag = lp_api_lite.get_most_recent_tag(tags, the_branch)
-        if best_tag is None:
-            best_message = ''
-        else:
-            best_message = '\nThe most recent tag found is %s' % (best_tag,)
-        trace.warning(
-            'Packaging branch is not up-to-date. The most recent published\n'
-            'version in %s is %s, but it is not in the branch tags for:\n  %s%s'
-            % (place, latest_ver, the_branch.base, best_message))
+    lp_api_lite.report_freshness(the_branch, verbosity, latest_pub)
+
 
 def _register_hooks():
     _mod_branch.Branch.hooks.install_named_hook('open',

=== modified file 'bzrlib/plugins/launchpad/lp_api_lite.py'
--- bzrlib/plugins/launchpad/lp_api_lite.py	2011-07-18 16:51:56 +0000
+++ bzrlib/plugins/launchpad/lp_api_lite.py	2011-07-22 12:04:10 +0000
@@ -33,6 +33,7 @@
     except ImportError:
         json = None
 
+import time
 import urllib
 import urllib2
 
@@ -156,6 +157,21 @@
             trace.log_exception_quietly()
             return None
 
+    def place(self):
+        """Text-form for what location this represents.
+
+        Example::
+            ubuntu, natty => Ubuntu Natty
+            ubuntu, natty-proposed => Ubuntu Natty Proposed
+        :return: A string representing the location we are checking.
+        """
+        place = self._archive
+        if self._series is not None:
+            place = '%s %s' % (place, self._series)
+        if self._pocket is not None and self._pocket != 'Release':
+            place = '%s %s' % (place, self._pocket)
+        return place.title()
+
 
 def get_latest_publication(archive, series, project):
     """Get the most recent publication for a given project.
@@ -187,3 +203,84 @@
                 return reverse_dict[rev_id]
     finally:
         the_branch.unlock()
+
+
+def _get_newest_versions(the_branch, latest_pub):
+    """Get information about how 'fresh' this packaging branch is.
+
+    :param the_branch: The Branch to check
+    :param latest_pub: The LatestPublication used to check most recent
+        published version.
+    :return: (latest_ver, branch_latest_ver)
+    """
+    t = time.time()
+    latest_ver = latest_pub.get_latest_version()
+    t_latest_ver = time.time() - t
+    trace.mutter('LatestPublication.get_latest_version took: %.3fs'
+                 % (t_latest_ver,))
+    if latest_ver is None:
+        return None, None
+    t = time.time()
+    tags = the_branch.tags.get_tag_dict()
+    t_tag_dict = time.time() - t
+    trace.mutter('LatestPublication.get_tag_dict took: %.3fs' % (t_tag_dict,))
+    if latest_ver in tags:
+        # branch might have a newer tag, but we don't really care
+        return latest_ver, latest_ver
+    else:
+        best_tag = get_most_recent_tag(tags, the_branch)
+        return latest_ver, best_tag
+
+
+def _report_freshness(latest_ver, branch_latest_ver, place, verbosity,
+                      report_func):
+    """Report if the branch is up-to-date."""
+    if latest_ver is None:
+        if verbosity == 'all':
+            report_func('Most recent %s version: MISSING' % (place,))
+        elif verbosity == 'short':
+            report_func('%s is MISSING a version' % (place,))
+        return
+    elif latest_ver == branch_latest_ver:
+        if verbosity == 'minimal':
+            return
+        elif verbosity == 'short':
+            report_func('%s is CURRENT in %s' % (latest_ver, place))
+        else:
+            report_func('Most recent %s version: %s\n'
+                       'Packaging branch status: CURRENT'
+                       % (place, latest_ver))
+    else:
+        if verbosity in ('minimal', 'short'):
+            if branch_latest_ver is None:
+                branch_latest_ver = 'Branch'
+            report_func('%s is OUT-OF-DATE, %s has %s'
+                        % (branch_latest_ver, place, latest_ver))
+        else:
+            report_func('Most recent %s version: %s\n'
+                        'Packaging branch version: %s\n'
+                        'Packaging branch status: OUT-OF-DATE'
+                        % (place, latest_ver, branch_latest_ver))
+
+
+def report_freshness(the_branch, verbosity, latest_pub):
+    """Report to the user how up-to-date the packaging branch is.
+
+    :param the_branch: A Branch object
+    :param verbosity: Can be one of:
+        off: Do not print anything, and skip all checks.
+        all: Print all information that we have in a verbose manner, this
+             includes misses, etc.
+        short: Print information, but only one-line summaries
+        minimal: Only print a one-line summary when the package branch is
+                 out-of-date
+    :param latest_pub: A LatestPublication instance
+    """
+    if verbosity == 'off':
+        return
+    if verbosity is None:
+        verbosity = 'all'
+    latest_ver, branch_ver = _get_newest_versions(the_branch, latest_pub)
+    place = latest_pub.place()
+    _report_freshness(latest_ver, branch_ver, place, verbosity,
+                      trace.note)

=== modified file 'bzrlib/plugins/launchpad/test_lp_api_lite.py'
--- bzrlib/plugins/launchpad/test_lp_api_lite.py	2011-07-18 16:51:56 +0000
+++ bzrlib/plugins/launchpad/test_lp_api_lite.py	2011-07-22 12:04:10 +0000
@@ -17,12 +17,16 @@
 """Tools for dealing with the Launchpad API without using launchpadlib.
 """
 
+import doctest
 import socket
 
 from bzrlib import tests
 from bzrlib.plugins import launchpad
 from bzrlib.plugins.launchpad import lp_api_lite
 
+from testtools.matchers import DocTestMatches
+
+
 class _JSONParserFeature(tests.Feature):
 
     def _probe(self):
@@ -76,6 +80,10 @@
                                 project='bzr'):
         return lp_api_lite.LatestPublication(archive, series, project)
 
+    def assertPlace(self, place, archive, series, project):
+        lp = lp_api_lite.LatestPublication(archive, series, project)
+        self.assertEqual(place, lp.place())
+
     def test_init(self):
         latest_pub = self.make_latest_publication()
         self.assertEqual('ubuntu', latest_pub._archive)
@@ -262,6 +270,14 @@
         latest_pub = self.make_latest_publication()
         self.assertIsNot(None, latest_pub.get_latest_version())
 
+    def test_place(self):
+        self.assertPlace('Ubuntu', 'ubuntu', None, 'bzr')
+        self.assertPlace('Ubuntu Natty', 'ubuntu', 'natty', 'bzr')
+        self.assertPlace('Ubuntu Natty Proposed', 'ubuntu', 'natty-proposed',
+                         'bzr')
+        self.assertPlace('Debian', 'debian', None, 'bzr')
+        self.assertPlace('Debian Sid', 'debian', 'sid', 'bzr')
+
 
 class TestIsUpToDate(tests.TestCase):
 
@@ -348,3 +364,180 @@
         self.assertEqual('B', b.last_revision())
         self.assertEqual('tip-1.0',
                          lp_api_lite.get_most_recent_tag(tag_dict, b))
+
+
+class StubLatestPublication(object):
+
+    def __init__(self, latest):
+        self.called = False
+        self.latest = latest
+
+    def get_latest_version(self):
+        self.called = True
+        return self.latest
+
+    def place(self):
+        return 'Ubuntu Natty'
+
+
+class TestReportFreshness(tests.TestCaseWithMemoryTransport):
+
+    def setUp(self):
+        super(TestReportFreshness, self).setUp()
+        builder = self.make_branch_builder('tip')
+        builder.build_snapshot('A', [], [
+            ('add', ('', 'root-id', 'directory', None))])
+        self.branch = builder.get_branch()
+
+    def assertFreshnessReports(self, verbosity, latest_version, content):
+        """Assert that lp_api_lite.report_freshness reports the given content.
+
+        :param verbosity: The reporting level
+        :param latest_version: The version reported by StubLatestPublication
+        :param content: The expected content. This should be in DocTest form.
+        """
+        orig_log_len = len(self.get_log())
+        lp_api_lite.report_freshness(self.branch, verbosity,
+            StubLatestPublication(latest_version))
+        new_content = self.get_log()[orig_log_len:]
+        # Strip out lines that have LatestPublication.get_* because those are
+        # timing related lines. While interesting to log for now, they aren't
+        # something we want to be testing
+        new_content = new_content.split('\n')
+        for i in range(2):
+            if (len(new_content) > 0
+                and 'LatestPublication.get_' in new_content[0]):
+                new_content = new_content[1:]
+        new_content = '\n'.join(new_content)
+        self.assertThat(new_content,
+            DocTestMatches(content,
+                doctest.ELLIPSIS | doctest.REPORT_UDIFF))
+
+    def test_verbosity_off_skips_check(self):
+        # We force _get_package_branch_info so that we know it would otherwise
+        # try to connect to launcphad
+        self.overrideAttr(launchpad, '_get_package_branch_info',
+            lambda x: ('ubuntu', 'natty', 'bzr'))
+        self.overrideAttr(lp_api_lite, 'LatestPublication',
+            lambda *args: self.fail('Tried to query launchpad'))
+        c = self.branch.get_config()
+        c.set_user_option('launchpad.packaging_verbosity', 'off')
+        orig_log_len = len(self.get_log())
+        launchpad._check_is_up_to_date(self.branch)
+        new_content = self.get_log()[orig_log_len:]
+        self.assertContainsRe(new_content,
+            'not checking memory.*/tip/ because verbosity is turned off')
+
+    def test_verbosity_off(self):
+        latest_pub = StubLatestPublication('1.0-1ubuntu2')
+        lp_api_lite.report_freshness(self.branch, 'off', latest_pub)
+        self.assertFalse(latest_pub.called)
+
+    def test_verbosity_all_out_of_date_smoke(self):
+        self.branch.tags.set_tag('1.0-1ubuntu1', 'A')
+        self.assertFreshnessReports('all', '1.0-1ubuntu2',
+             '    INFO  Most recent Ubuntu Natty version: 1.0-1ubuntu2\n'
+             'Packaging branch version: 1.0-1ubuntu1\n'
+             'Packaging branch status: OUT-OF-DATE\n')
+
+
+class Test_GetNewestVersions(tests.TestCaseWithMemoryTransport):
+
+    def setUp(self):
+        super(Test_GetNewestVersions, self).setUp()
+        builder = self.make_branch_builder('tip')
+        builder.build_snapshot('A', [], [
+            ('add', ('', 'root-id', 'directory', None))])
+        self.branch = builder.get_branch()
+
+    def assertLatestVersions(self, latest_branch_version, pub_version):
+        if latest_branch_version is not None:
+            self.branch.tags.set_tag(latest_branch_version, 'A')
+        latest_pub = StubLatestPublication(pub_version)
+        self.assertEqual((pub_version, latest_branch_version),
+            lp_api_lite._get_newest_versions(self.branch, latest_pub))
+
+    def test_no_tags(self):
+        self.assertLatestVersions(None, '1.0-1ubuntu2')
+
+    def test_out_of_date(self):
+        self.assertLatestVersions('1.0-1ubuntu1', '1.0-1ubuntu2')
+
+    def test_up_to_date(self):
+        self.assertLatestVersions('1.0-1ubuntu2', '1.0-1ubuntu2')
+
+    def test_missing(self):
+        self.assertLatestVersions(None, None)
+
+
+class Test_ReportFreshness(tests.TestCase):
+
+    def assertReportedFreshness(self, verbosity, latest_ver, branch_latest_ver,
+                               content, place='Ubuntu Natty'):
+        """Assert that lp_api_lite.report_freshness reports the given content.
+        """
+        reported = []
+        def report_func(value):
+            reported.append(value)
+        lp_api_lite._report_freshness(latest_ver, branch_latest_ver, place,
+                                      verbosity, report_func)
+        new_content = '\n'.join(reported)
+        self.assertThat(new_content,
+            DocTestMatches(content,
+                doctest.ELLIPSIS | doctest.REPORT_UDIFF))
+
+    def test_verbosity_minimal_no_tags(self):
+        self.assertReportedFreshness('minimal', '1.0-1ubuntu2', None,
+            'Branch is OUT-OF-DATE, Ubuntu Natty has 1.0-1ubuntu2\n')
+
+    def test_verbosity_minimal_out_of_date(self):
+        self.assertReportedFreshness('minimal', '1.0-1ubuntu2', '1.0-1ubuntu1',
+            '1.0-1ubuntu1 is OUT-OF-DATE,'
+            ' Ubuntu Natty has 1.0-1ubuntu2\n')
+
+    def test_verbosity_minimal_up_to_date(self):
+        self.assertReportedFreshness('minimal', '1.0-1ubuntu2', '1.0-1ubuntu2',
+             '')
+
+    def test_verbosity_minimal_missing(self):
+        self.assertReportedFreshness('minimal', None, None,
+             '')
+
+    def test_verbosity_short_out_of_date(self):
+        self.assertReportedFreshness('short', '1.0-1ubuntu2', '1.0-1ubuntu1',
+            '1.0-1ubuntu1 is OUT-OF-DATE,'
+            ' Ubuntu Natty has 1.0-1ubuntu2\n')
+
+    def test_verbosity_short_up_to_date(self):
+        self.assertReportedFreshness('short', '1.0-1ubuntu2', '1.0-1ubuntu2',
+             '1.0-1ubuntu2 is CURRENT in Ubuntu Natty')
+
+    def test_verbosity_short_missing(self):
+        self.assertReportedFreshness('short', None, None,
+             'Ubuntu Natty is MISSING a version')
+
+    def test_verbosity_all_no_tags(self):
+        self.assertReportedFreshness('all', '1.0-1ubuntu2', None,
+             'Most recent Ubuntu Natty version: 1.0-1ubuntu2\n'
+             'Packaging branch version: None\n'
+             'Packaging branch status: OUT-OF-DATE\n')
+
+    def test_verbosity_all_out_of_date(self):
+        self.assertReportedFreshness('all', '1.0-1ubuntu2', '1.0-1ubuntu1',
+             'Most recent Ubuntu Natty version: 1.0-1ubuntu2\n'
+             'Packaging branch version: 1.0-1ubuntu1\n'
+             'Packaging branch status: OUT-OF-DATE\n')
+
+    def test_verbosity_all_up_to_date(self):
+        self.assertReportedFreshness('all', '1.0-1ubuntu2', '1.0-1ubuntu2',
+             'Most recent Ubuntu Natty version: 1.0-1ubuntu2\n'
+             'Packaging branch status: CURRENT\n')
+
+    def test_verbosity_all_missing(self):
+        self.assertReportedFreshness('all', None, None,
+             'Most recent Ubuntu Natty version: MISSING\n')
+
+    def test_verbosity_None_is_all(self):
+        self.assertReportedFreshness(None, '1.0-1ubuntu2', '1.0-1ubuntu2',
+             'Most recent Ubuntu Natty version: 1.0-1ubuntu2\n'
+             'Packaging branch status: CURRENT\n')

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-07-21 07:08:05 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-07-22 12:04:10 +0000
@@ -24,7 +24,25 @@
   checks to see if the most recent published source package version for
   that project is present in the branch tags. This should help developers
   trust whether the packaging branch is up-to-date and can be used for new
-  changes. (John Arbash Meinel, #609187)
+  changes. The level of verbosity is controlled by the config item
+  ``bzr.plugins.launchpad.packaging_verbosity``. It can be set to one of
+
+  off
+    disable all checks
+
+
+  minimal
+    only display if the branch is out-of-date
+
+  short
+    also display single-line up-to-date and missing,
+
+
+  all
+    (default) display multi-line content for all states
+
+
+  (John Arbash Meinel, #609187, #812928)
 
 * Add a config option gpg_signature_key for setting which GPG key
   should be used to sign commits. Also default to using the gpg user