1071432 - bzpost should be more careful about which bugs it updates

Status: RESOLVED FIXED

(Developer Services :: Mercurial: bzpost, defect)

Description

[Matthew Gregan [:kinetik]]

I pulled the latest mozilla-inbound, qpushed two changesets to test with no bug numbers in the commit summary, then pushed my changesets to try.  Along with the two changesets of mine, a new changeset from m-i was included.  bzpost selected the bug from this changeset (not owned by me) to update.

Would something like excluding all outgoing changesets that are already in the "public" phase be sufficient?

FWIW, my try push was https://tbpl.mozilla.org/?tree=Try&rev=97bc8d57e157

And the bug bzpost updated was jesup's bug 1069646 from cset 148862fe121e.

Comment 1

[Gregory Szorc [:gps]]

Good catch.

I think ignoring public changesets from try pushes is a good idea.

Comment 2

[Gregory Szorc [:gps]]

Attachment: bzpost: don't consider public changesets when pushing to Try ()

Pushing changesets to Try could push logically unrelated changesets that
were recently pushed/pulled to a Firefox repo. Before this patch, if the
"real" changesets pushed to Try didn't contain a bug number, we could
inherit a bug number from one of these public changesets and associate
the Try push with that bug. That behavior was wrong.

This patch fixes the problem by ignoring public changesets during Try
pushes.

Comment 3

[Mike Hommey [:glandium] OoO until Jun 18]

Comment on attachment 8497026 [details] [diff] [review]
bzpost: don't consider public changesets when pushing to Try ()

Review of attachment 8497026 [details] [diff] [review]:
-----------------------------------------------------------------

::: hgext/bzpost/tests/test-post.t
@@ +63,5 @@
> +  searching for changes
> +  remote: adding changesets
> +  remote: adding manifests
> +  remote: adding file changes
> +  remote: added 3 changesets with 3 changes to 1 files

You're not pushing with a bug number, so you're not "recording TBPL push in bug xxx". Also, the existing test doesn't check what's being posted in bug 123 afaict.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8497026 [details] [diff] [review]
bzpost: don't consider public changesets when pushing to Try ()

Yes, I am pushing with a bug number:

   $ echo try > foo
   $ hg commit -m 'try: -b do -p all -u all -t all'
   $ hg out http://localhost:$HGPORT/try
+  comparing with http://localhost:$HGPORT/try
+  searching for changes
+  changeset:   0:3d7d3272d708
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     Bug 123 - Add foo
+
+  changeset:   1:dad9f7f3ddf8
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     New draft
+
+  changeset:   2:496931107fa6
+  tag:         tip
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     try: -b do -p all -u all -t all
+
   $ hg push --force http://localhost:$HGPORT/try
   pushing to http://localhost:$HGPORT/try
   searching for changes
   remote: adding changesets
   remote: adding manifests
   remote: adding file changes
   remote: added 3 changesets with 3 changes to 1 files

The patch changes behavior seen in https://hg.mozilla.org/try/pushloghtml?changeset=97bc8d57e157: if the subset of commits relevant to the try push (commits in draft phase) don't contain a reference to a bug, the Try push shouldn't be associated by bugs referenced by public changesets at the "bottom of the push." The old code was greedy. The new code isn't.

Since the "try relevant" commits don't contain a bug reference, the correct behavior (as implemented) is for bzpost to no-op.

I believe this patch and behavior is correct and am re-requesting review. If I need to better clarify what's happening in the code comments, that's fair review feedback.

Comment 5

[Gregory Szorc [:gps]]

Also, you are correct the existing tests don't explicitly test what's being posted in Bugzilla. That's because the outgoing HTTP requests are being mocked. In the ideal world, we'd start up a local BMO instance as part of tests. We don't do this because running a BMO instance is sufficiently complex (lots of Perl dependencies, MySQL database, Apache, etc). We've survived being lazy and mocking out Bugzilla. However, ReviewBoard is forcing my hand, so I'll be working on spinning up a BMO instance for testing this week. We can hopefully soon move the bzpost tests to use a real Bugzilla. Of course, running a server-side service for posting Bugzilla updates is preferred. I hope to get that on the Developer Services roadmap.

Comment 6

[Mike Hommey [:glandium] OoO until Jun 18]

(In reply to Gregory Szorc [:gps] from comment #4)
> Comment on attachment 8497026 [details] [diff] [review]
> bzpost: don't consider public changesets when pushing to Try ()
> 
> Yes, I am pushing with a bug number:
> 
>    $ echo try > foo
>    $ hg commit -m 'try: -b do -p all -u all -t all'
>    $ hg out http://localhost:$HGPORT/try
> +  comparing with http://localhost:$HGPORT/try
> +  searching for changes
> +  changeset:   0:3d7d3272d708
> +  user:        test
> +  date:        Thu Jan 01 00:00:00 1970 +0000
> +  summary:     Bug 123 - Add foo
> +
> +  changeset:   1:dad9f7f3ddf8
> +  user:        test
> +  date:        Thu Jan 01 00:00:00 1970 +0000
> +  summary:     New draft
> +
> +  changeset:   2:496931107fa6
> +  tag:         tip
> +  user:        test
> +  date:        Thu Jan 01 00:00:00 1970 +0000
> +  summary:     try: -b do -p all -u all -t all
> +
>    $ hg push --force http://localhost:$HGPORT/try
>    pushing to http://localhost:$HGPORT/try
>    searching for changes
>    remote: adding changesets
>    remote: adding manifests
>    remote: adding file changes
>    remote: added 3 changesets with 3 changes to 1 files
> 
> The patch changes behavior seen in
> https://hg.mozilla.org/try/pushloghtml?changeset=97bc8d57e157: if the subset
> of commits relevant to the try push (commits in draft phase) don't contain a
> reference to a bug, the Try push shouldn't be associated by bugs referenced
> by public changesets at the "bottom of the push." The old code was greedy.
> The new code isn't.

My point is that you should be testing that bug 123 is not updated but bug xxx, which would be referenced in the "New draft" changeset is (btw, it would probably be better if there was also a test with more than one bug being updated).

(In reply to Gregory Szorc [:gps] from comment #5)
> Also, you are correct the existing tests don't explicitly test what's being
> posted in Bugzilla. That's because the outgoing HTTP requests are being
> mocked. In the ideal world, we'd start up a local BMO instance as part of
> tests. We don't do this because running a BMO instance is sufficiently
> complex (lots of Perl dependencies, MySQL database, Apache, etc). We've
> survived being lazy and mocking out Bugzilla. However, ReviewBoard is
> forcing my hand, so I'll be working on spinning up a BMO instance for
> testing this week. We can hopefully soon move the bzpost tests to use a real
> Bugzilla. Of course, running a server-side service for posting Bugzilla
> updates is preferred. I hope to get that on the Developer Services roadmap.

You don't need a complete bugzilla instance to check what content is being sent to the fake server.

Comment 7

[Gregory Szorc [:gps]]

Attachment: bzpost: don't consider public changesets when pushing to Try ()

I spent a little extra effort and implemented the testing robustness
that you seek.

Comment 8

[Mike Hommey [:glandium] OoO until Jun 18]

Comment on attachment 8497604 [details] [diff] [review]
bzpost: don't consider public changesets when pushing to Try ()

Review of attachment 8497604 [details] [diff] [review]:
-----------------------------------------------------------------

::: hgext/bzpost/__init__.py
@@ +69,5 @@
> +
> +    A proper solution to avoid this hack involves deep hooks into the Bugzilla
> +    client module to intercept the outgoing HTTP request. Even better would be
> +    running a real Bugzilla server as part of tests. Until then, we live with
> +    this hack.

Alternatively you could mock bugsy.

@@ +71,5 @@
> +    client module to intercept the outgoing HTTP request. Even better would be
> +    running a real Bugzilla server as part of tests. Until then, we live with
> +    this hack.
> +    '''
> +    if 'BZPOST_COMMENT_DIRECTORY' in os.environ:

Could have used a ui.config.

@@ +74,5 @@
> +    '''
> +    if 'BZPOST_COMMENT_DIRECTORY' in os.environ:
> +        p = os.path.join(os.environ['BZPOST_COMMENT_DIRECTORY'], str(bug))
> +        with open(p, 'wb') as fh:
> +            fh.write(comment)

Why not just write it with ui.write and have it directly in the test output?

::: hgext/bzpost/tests/test-post.t
@@ +144,5 @@
> +  $ ls comments
> +  123
> +  $ cat comments/123
> +  https://tbpl.mozilla.org/?tree=Try&rev=8ddc73283ce3 (no-eol)
> +  $ rm comments/*

Missing the case where one pushes several bugs to try (that is, all of them being draft).

Comment 9

[Gregory Szorc [:gps]]

Attachment: bzpost: don't consider public changesets when pushing to Try ()

I switched from environment variables to hgrc variable and am having it
write to stdout. I also added a test for multiple commits, multiple
bugs.

Comment 10

[Mike Hommey [:glandium] OoO until Jun 18]

Comment on attachment 8498603 [details] [diff] [review]
bzpost: don't consider public changesets when pushing to Try ()

Review of attachment 8498603 [details] [diff] [review]:
-----------------------------------------------------------------

::: hgext/bzpost/__init__.py
@@ +77,5 @@
> +
> +    lines = ['posting to bug %s' % bug]
> +    lines.extend(comment.splitlines())
> +    for line in lines:
> +        ui.write(' %s\n' % line)

I'd add at least one space to make the test output a little nicer.

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/1e900584cb2d

Comment 12

[Gregory Szorc [:gps]]

With indentation fix.