Bug 106754 - Sheriffbot command aliases need test cases
Summary: Sheriffbot command aliases need test cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alan Cutter
URL:
Keywords:
Depends on:
Blocks: 106628
  Show dependency treegraph
 
Reported: 2013-01-13 21:28 PST by Alan Cutter
Modified: 2013-01-14 01:31 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.05 KB, patch)
2013-01-13 22:02 PST, Alan Cutter
no flags Details | Formatted Diff | Diff
Patch (2.64 KB, patch)
2013-01-13 23:35 PST, Alan Cutter
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alan Cutter 2013-01-13 21:28:22 PST
Currently there are no automated tests for Sheriffbot command aliases.
Comment 1 Alan Cutter 2013-01-13 22:02:28 PST
Created attachment 182506 [details]
Patch
Comment 2 Steve Block 2013-01-13 22:09:25 PST
Comment on attachment 182506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182506&action=review

> Tools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py:46
> +        self.assertEqual(command, Sheriffs)

Do you intend to add tests for the other sheriffbot commands too?
Comment 3 Alan Cutter 2013-01-13 22:14:21 PST
(In reply to comment #2)
> (From update of attachment 182506 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182506&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py:46
> > +        self.assertEqual(command, Sheriffs)
> 
> Do you intend to add tests for the other sheriffbot commands too?

Yes, the other aliases had not landed at the time of writing. They have now so I'll add them.
Comment 4 Eric Seidel (no email) 2013-01-13 22:30:07 PST
Comment on attachment 182506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182506&action=review

> Tools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py:43
> +        tool = MockTool()
> +        options = MockOptions()
> +        options.ensure_value("confirm", False)
> +        sheriffbot = SheriffBot()
> +        sheriffbot.execute(options, [], tool, MockQueueEngine)
> +        sheriffbot.begin_work_queue()
> +        irc_bot = sheriffbot._irc_bot

Quite an epic amount of boiler plate. :)
Comment 5 Alan Cutter 2013-01-13 23:35:15 PST
Created attachment 182518 [details]
Patch
Comment 6 Eric Seidel (no email) 2013-01-13 23:56:08 PST
Comment on attachment 182518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182518&action=review

> Tools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py:42
> +        tool = MockTool()
> +        options = MockOptions()
> +        options.ensure_value("confirm", False)
> +        sheriffbot = SheriffBot()
> +        sheriffbot.execute(options, [], tool, MockQueueEngine)
> +        sheriffbot.begin_work_queue()

Next time we add tests, this should probably move to a setUp() method (which is automatically called for you)

> Tools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py:46
> +        revert_command, args = irc_bot._parse_command_and_args("revert")
> +        self.assertEqual(revert_command, Rollout)

I often add a _assertCommandNamed(name, command) or similar helper method when the asserts take more than one line to execute.
Comment 7 Alan Cutter 2013-01-14 01:14:58 PST
(In reply to comment #6)
> (From update of attachment 182518 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182518&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py:42
> > +        tool = MockTool()
> > +        options = MockOptions()
> > +        options.ensure_value("confirm", False)
> > +        sheriffbot = SheriffBot()
> > +        sheriffbot.execute(options, [], tool, MockQueueEngine)
> > +        sheriffbot.begin_work_queue()
> 
> Next time we add tests, this should probably move to a setUp() method (which is automatically called for you)
> 
> > Tools/Scripts/webkitpy/tool/commands/sheriffbot_unittest.py:46
> > +        revert_command, args = irc_bot._parse_command_and_args("revert")
> > +        self.assertEqual(revert_command, Rollout)
> 
> I often add a _assertCommandNamed(name, command) or similar helper method when the asserts take more than one line to execute.

Ah yes, I did notice that pattern in the EWS tests. Next change to Sheriffbot will come with more abstraction in the tests.
Comment 8 Eric Seidel (no email) 2013-01-14 01:24:54 PST
Comment on attachment 182518 [details]
Patch

Thanks agian.
Comment 9 WebKit Review Bot 2013-01-14 01:31:28 PST
Comment on attachment 182518 [details]
Patch

Clearing flags on attachment: 182518

Committed r139592: <http://trac.webkit.org/changeset/139592>
Comment 10 WebKit Review Bot 2013-01-14 01:31:32 PST
All reviewed patches have been landed.  Closing bug.