Bug 137303 - commitqueuetasks_unittest references a non-existent member variable when mock-reporting flakey tests.
Summary: commitqueuetasks_unittest references a non-existent member variable when mock...
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-01 10:08 PDT by Jake Nielsen
Modified: 2014-10-02 09:56 PDT (History)
3 users (show)

See Also:


Attachments
Makes the appropriate name change (2.19 KB, patch)
2014-10-01 10:18 PDT, Jake Nielsen
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff
Adds a unit test to exercise the mock (4.62 KB, patch)
2014-10-01 14:01 PDT, Jake Nielsen
dbates: review+
dbates: commit-queue-
Details | Formatted Diff | Diff
typo fixed (4.62 KB, patch)
2014-10-01 14:58 PDT, Jake Nielsen
no flags Details | Formatted Diff | Diff
typo *really* fixed. (4.62 KB, patch)
2014-10-01 15:02 PDT, Jake Nielsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jake Nielsen 2014-10-01 10:08:01 PDT
MockCommitQueue tries to access the "filename" member of a TestResult in unexcercised code.
Comment 1 Jake Nielsen 2014-10-01 10:18:52 PDT
Created attachment 239033 [details]
Makes the appropriate name change
Comment 2 Daniel Bates 2014-10-01 12:51:25 PDT
Comment on attachment 239033 [details]
Makes the appropriate name change

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

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:80
>      def report_flaky_tests(self, patch, flaky_results, results_archive):
> -        flaky_tests = [result.filename for result in flaky_results]
> +        flaky_tests = [result.test_name for result in flaky_results]
>          _log.info("report_flaky_tests: patch='%s' flaky_tests='%s' archive='%s'" % (patch.id(), flaky_tests, results_archive.filename))

How did you come to the decision to fix this code as opposed to removing it? I mean, you mentioned in comment #0 that this is "unexcercised [sic] code". We should not keep unused code in the tree.
Comment 3 Jake Nielsen 2014-10-01 13:19:21 PDT
(In reply to comment #2)
> (From update of attachment 239033 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239033&action=review
> 
> > Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:80
> >      def report_flaky_tests(self, patch, flaky_results, results_archive):
> > -        flaky_tests = [result.filename for result in flaky_results]
> > +        flaky_tests = [result.test_name for result in flaky_results]
> >          _log.info("report_flaky_tests: patch='%s' flaky_tests='%s' archive='%s'" % (patch.id(), flaky_tests, results_archive.filename))
> 
> How did you come to the decision to fix this code as opposed to removing it? I mean, you mentioned in comment #0 that this is "unexcercised [sic] code". We should not keep unused code in the tree.

I found this bug because one of the changes I was going to make exercised this code. Although it's not exercised as of this moment, it likely will be in the near future. Probably by me.
Comment 4 Daniel Bates 2014-10-01 13:45:01 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 239033 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=239033&action=review
> > 
> > > Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:80
> > >      def report_flaky_tests(self, patch, flaky_results, results_archive):
> > > -        flaky_tests = [result.filename for result in flaky_results]
> > > +        flaky_tests = [result.test_name for result in flaky_results]
> > >          _log.info("report_flaky_tests: patch='%s' flaky_tests='%s' archive='%s'" % (patch.id(), flaky_tests, results_archive.filename))
> > 
> > How did you come to the decision to fix this code as opposed to removing it? I mean, you mentioned in comment #0 that this is "unexcercised [sic] code". We should not keep unused code in the tree.
> 
> I found this bug because one of the changes I was going to make exercised this code. Although it's not exercised as of this moment, it likely will be in the near future. Probably by me.

As mentioned in-person today (10/01), we should either add a test for this change (to ensure we don't break it again) or remove this code (we can always add an implementation for this method or add back the entire method when we actually make use of it)
Comment 5 Daniel Bates 2014-10-01 13:46:00 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 239033 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=239033&action=review
> > > 
> > > > Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:80
> > > >      def report_flaky_tests(self, patch, flaky_results, results_archive):
> > > > -        flaky_tests = [result.filename for result in flaky_results]
> > > > +        flaky_tests = [result.test_name for result in flaky_results]
> > > >          _log.info("report_flaky_tests: patch='%s' flaky_tests='%s' archive='%s'" % (patch.id(), flaky_tests, results_archive.filename))
> > > 
> > > How did you come to the decision to fix this code as opposed to removing it? I mean, you mentioned in comment #0 that this is "unexcercised [sic] code". We should not keep unused code in the tree.
> > 
> > I found this bug because one of the changes I was going to make exercised this code. Although it's not exercised as of this moment, it likely will be in the near future. Probably by me.
> 
> As mentioned in-person today (10/01), we should either add a test for this change (to ensure we don't break it again) or remove this code (we can always add an implementation for this method or add back the entire method when we actually make use of it)

*...we can always add a non-trivial implementation...
Comment 6 Jake Nielsen 2014-10-01 14:01:59 PDT
Created attachment 239048 [details]
Adds a unit test to exercise the mock
Comment 7 Daniel Bates 2014-10-01 14:28:06 PDT
Comment on attachment 239048 [details]
Adds a unit test to exercise the mock

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

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:338
> +    def test_simple_flakey_test_failure(self):

Nit: flakey => flaky

For consistency with the majority of the Python code, including the naming of the function report_flaky_tests() and local variable flaky_tests, we should use "flaky" instead of "flakey". We should look to fix up the few places in the Python code where we use the word "flakey".
Comment 8 Jake Nielsen 2014-10-01 14:58:00 PDT
Created attachment 239051 [details]
typo fixed
Comment 9 Jake Nielsen 2014-10-01 14:58:50 PDT
Hold up, I uploaded the wrong patch.
Comment 10 Jake Nielsen 2014-10-01 15:02:49 PDT
Created attachment 239053 [details]
typo *really* fixed.
Comment 11 WebKit Commit Bot 2014-10-01 15:46:43 PDT
Comment on attachment 239053 [details]
typo *really* fixed.

Clearing flags on attachment: 239053

Committed r174182: <http://trac.webkit.org/changeset/174182>
Comment 12 WebKit Commit Bot 2014-10-01 15:46:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Commit Bot 2014-10-01 23:17:07 PDT
Re-opened since this is blocked by bug 137334
Comment 14 Daniel Bates 2014-10-02 09:56:06 PDT
(In reply to comment #13)
> Re-opened since this is blocked by bug 137334

Disregard this message. Bug #137334 was inadvertently marked as being blocked by this bug (bug #137303).