[MERGE] Initial support for marking bugs as fixed using bzr

[Jonathan Lange]

On 4/19/07, Robert Collins <robertc at robertcollins.net> wrote:
> On Fri, 2007-04-13 at 17:33 +1000, Jonathan Lange wrote:
>
> --- /dev/null
> +++ bzrlib/bugtracker.py
> @@ -0,0 +1,148 @@
>
> ...
> ....+
> +Thus, this module provides a registry of types of bug tracker (e.g. Launchpad,
> +Trac). Given a short-hand tag (e.g. 'lp', 'twisted') and a branch with
> +configuration information, these tracker types can return an instance capable
> +of converting bug IDs into URLs.
> +"""
>
> I think referring to these as 'tags' is potentially confusing. Branch
> have tags. Chatting on IRC we came up with
> 'abbreviated_bugtracker_name'. While this is a mouthful, its clear what
> it refers to. I'd like you to look for 'tag' throughout this file and
> use a more precise name that isn't overloaded within bzrlib already.
> E.g. withing the tracker classes, perhaps 'abbreviation' is a good
> attribute name.
>

Changed to abbreviatied_bugtracker_name and abbreviation in code and docs.

> +
> +def get_bug_url(tag, branch, bug_id):
> +    """Return a URL pointing to the bug identified by 'bug_id'."""
> +    return tracker_registry.get_tracker(tag, branch).get_bug_url(bug_id)
>
> 'pointing to the bug' is a little misleading - surely its pointing to
> the webpage for the bug. (That is, we're not using URN style
> identification of the bug).
>

Changed.

> ...
> +        for tracker_name, tracker_type in self.iteritems():
> +            tracker = tracker_type.get(tag, branch)
> +            if tracker is not None:
> +                return tracker
> +        raise KeyError("No tracker found for %r on %r" % (tag, branch))
>
> tracker_name is not used here. Why not use self.values() ? (Or isn't that defined).

values() isn't defined, and neither is __getitem__. I'll file a bug.

> +
> +tracker_registry = TrackerRegistry()
> +"""Registry of bug trackers."""
>
> Perhaps this docstring is better served by having a __doc__ on instances
> of TrackerRegistry - wouldn't that work for pydoc?
>

I copied what's done in other modules in bzrlib. I haven't changed this.

> +
> +class UniqueBugTracker(object):
> +    """A style of bug tracker that exists in one place only, such as Launchpad.
> +
> +    If you have one of these trackers then subclass this and add attributes
> +    named 'tag' and 'base_url'. The former is the tag that the user will use
> +    on the command line. The latter is the url that the bug ids will be
> +    appended to.
> +
> +    If the bug_id must have a special form then override check_bug_id and
> +    raise an exception if the bug_id is not valid.
> +    """
>
> As a side note, launchpad doesn't strictly exist in only one place - the
> demo and staging servers have different databases to the production
> server and can thus talk about different bug numbers. I'm not sure it
> matters - but perhaps for testing it would be nice to be able to have
> throwaway branches that you use with e.g. staging, and have the right
> urls created.
>

I haven't really done anything about this.

> +...
>
> I like that you supply trackers for Launchpad, Debian, and Trac.
> Bugzilla is another common tracker and its a simple integer tracker but
> like trac appears at many urls. I think it would be good for the code
> factoring to implement Bugzilla urls too. (giving us two 'singleton'
> trackers, and two 'many instance' trackers).

Added Buzilla tracker.

> I dont like that you have subclasses for Launchpad and Debian that vary
> solely in parameters. That to me cries out for instances. This would in
> principal allow configuring abbreviations for any tracker that we have
> the basic facility for via branch.conf - or any other config file in the
> configuration system.

Yeah, I don't like it either. I've changed them to instances, but not
updated their config mechanism -- they are just registered manually.

> I am not making configuration via branch.conf a requirement for getting
> +1 - a bug report is fine. But I do think getting rid of the spurious
> classes is important and want that changed to get +1.
>
> +++ bzrlib/tests/test_bugtracker.py
> @@ -0,0 +1,182 @@
>
> +class TestGetBugURL(TestCaseWithMemoryTransport):
> +    """Tests for bugtracker.get_bug_url"""
> +
> +    def test_get_launchpad_url(self):
> +        """No matter the branch, lp:1234 should map to a Launchpad URL."""
> +        branch = self.make_branch('some_branch')
> +        self.assertEqual(
> +            'https://launchpad.net/bugs/1234',
> +            bugtracker.get_bug_url('lp', branch, '1234'))
>
> This test claims to test a negative, which isn't really possible. I
> think its more accurate to test something like 'The launchpad tracker
> does not consult the branch for determining its urls', and test that
> passing in 'None' as the branch results in the right URL.
>

This test has been replaced entirely, see below.

> +    def test_get_trac_url(self):
> +        trac_url = 'http://twistedmatrix.com/trac'
> +        branch = self.make_branch('some_branch')
> +        config = branch.get_config()
> +        config.set_user_option('trac_twisted_url', trac_url)
> +        self.assertEqual('%s/ticket/1234' % trac_url,
> +                         bugtracker.get_bug_url('twisted', branch, '1234'))
>
> This tests that one branch has the right url, but you need two to tell
> that the parameterisation works. E.g. change the config option and test
> the result changes.
>

Based on some of your comments further below, I've rewritten this set
of test cases. They now work by preparing a transient tracker. Your
review comments on these two tests were applied as necessary for the
new tests.

> +    def test_unrecognized_tag(self):
> +        """If the tag is unrecognized, then raise a KeyError."""
> +        branch = self.make_branch('some_branch')
> +        self.assertRaises(KeyError,
> +                          bugtracker.get_bug_url, 'xxx', branch, '1234')
>
> I think we should have a better error than KeyError - one that has a good format string for the UI.

I've created a new error and put tests for it into test_errors.

>
> ...
>
>
> +class TestUniqueIntegerBugTracker(TestCaseWithMemoryTransport):
> +
> +    def test_check_bug_id_only_accepts_integers(self):
> +        """An UniqueIntegerBugTracker only accepts integers as bug IDs."""
> +        tracker = bugtracker.UniqueIntegerBugTracker()
> +        self.assertEqual(None, tracker.check_bug_id('1234'))
>
> Is the return code relevant? I would have written this as two separate tests:
> one that does
> --
> tracker.check_bug_id('1234')
> --
> and one that does
> +        self.assertRaises(MalformedBugIdentifier, tracker.check_bug_id, 'red')
>

The return code is not relevant, but I have this twitch that flares up
when tests don't have calls to assert* methods. Changed.

>
> +class TestLaunchpadTracker(TestCaseWithMemoryTransport):
>
> I think all these tests can go with the refactoring I proposed earlier -
> all you need is a test that the launchpad_tracker is a registered
> instance with the right parameters.
>

Removed, replaced with thing that checks registration.

>
> +class TestDebianTracker(TestCaseWithMemoryTransport):
>
> Likewise.
>

Removed, replaced with thing that checks registration.

> +class TestTracTracker(TestCaseWithMemoryTransport):
> +    def setUp(self):
> +        TestCaseWithMemoryTransport.setUp(self)
> +        self.trac_url = 'http://twistedmatrix.com/trac'
> +
> +    def test_get_bug_url(self):
> +        """A TracTracker should map a Trac bug to a URL for that instance."""
> +        tracker = bugtracker.TracTracker(self.trac_url)
> +        self.assertEqual(
> +            '%s/ticket/1234' % self.trac_url, tracker.get_bug_url('1234'))
>
> This seems to be duplicated with the earlier trac test. I think the
> problem is that the first test is not clear about what its testing. Is
> it testing the convenience api or the underlying api. I think that as
> its claiming to test the convenience API, what you should be doing is
> registering transient bug trackers up there, and checking they get
> called into. That would reduce test duplication.
>

Changes made as recommended. See above comments for more details.

> --- NEWS
> +++ NEWS
> @@ -5,6 +5,9 @@
>      * Merge directives can now be supplied as input to `merge` and `pull`,
>        like bundles can.  (Aaron Bentley)
>
> +    * New option '--fixes' to commit, which stores bug fixing annotations as
> +      revision properties. (Jonathan Lange, James Henstridge, Robert Collins)
> +
>    INTERNALS:
>
>      * bzrlib API compatability with 0.8 has been dropped, cleaning up some
>
> There should be an entry here talking about the new API as well - as
> plugins can now add bug trackers.
>

I've added another entry to Improvements for the API. It's scant, but
will be easier for me to expand after receiving comments.

> --- bzrlib/builtins.py
> +++ bzrlib/builtins.py
>
> @@ -2074,6 +2074,8 @@
>                       Option('strict',
>                              help="refuse to commit if there are unknown "
>                              "files in the working tree."),
> +                     ListOption('fixes', type=str,
> +                                help="XXX -- fix a bug"),
>
> This needs fixing.

Fixed. I'm adding a help topic, as discussed on #bzr, but at the
moment, it's also pretty short. Feedback will help expand it.

> @@ -2083,8 +2085,32 @@
>                       ]
>      aliases = ['ci', 'checkin']
>
...
>
> === modified file bzrlib/errors.py // last-changed:jml at canonical.com-2007041305
> ... 1007-cq318kt5uj59k1ty
> --- bzrlib/errors.py
> +++ bzrlib/errors.py
> @@ -2046,3 +2046,12 @@
>
>      def __init__(self, tag_name):
>          self.tag_name = tag_name
> +
> +
> +class MalformedBugIdentifier(BzrError):
> +
> +    _fmt = "Did not understand bug identifier %(bug_id)s: %(reason)s"
> +
> +    def __init__(self, bug_id, reason):
> +        self.bug_id = bug_id
> +        self.reason = reason
>
> We have tests for error formatting in bzrlib/test_errors.py - can you add one please.

Added.

> --- bzrlib/option.py
> +++ bzrlib/option.py
> @@ -205,6 +205,26 @@
>          yield self.name, self.short_name(), argname, self.help
>
>
> +class ListOption(Option):
> +    def add_option(self, parser, short_name):
>
> class docstring please, and PEP8 vertical whitespace.
>
> IIRC Aaron had serious concerns about ListOption, have they been resolved?
>
> This also needs documentation in:
> doc/configuration.txt (for the trac bug tracker configuration). It might
> be nice to expand the tutorial (doc/tutorial.txt) to cover using this
> (perhaps using the launchpad tracker in the tutorial, as it does not
> need configuration).
>

Config docs added.

> I think theres a need for a 'managing a project with bzr' document - not
> one about centralised/decentralised etc, but about the features of bzr
> that are like this one, useful outside the strict 'I can commit code'
> space. Thats something I'll file a bug on though, its not needed for
> this to land.
>
> I'd like my points addressed and a re-review subsequently, because I
> think the incremental changes will be significant.
>

Points addressed. Thanks for the really helpful review.

cheers,
jml
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug-support.patch
Type: text/x-diff
Size: 615389 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070423/b298956f/attachment-0001.bin