Bug 137303 – commitqueuetasks_unittest references a non-existent member variable when mock-reporting flakey tests.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jake Nielsen]

MockCommitQueue tries to access the "filename" member of a TestResult in unexcercised code.

[Jake Nielsen]

Created attachment 239033 [details]
Makes the appropriate name change

[Daniel Bates]

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.

[Jake Nielsen]

(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.

[Daniel Bates]

(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)

[Daniel Bates]

(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...

[Jake Nielsen]

Created attachment 239048 [details]
Adds a unit test to exercise the mock

[Daniel Bates]

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".

[Jake Nielsen]

Created attachment 239051 [details]
typo fixed

[Jake Nielsen]

Hold up, I uploaded the wrong patch.

[Jake Nielsen]

Created attachment 239053 [details]
typo *really* fixed.

[WebKit Commit Bot]

Comment on attachment 239053 [details]
typo *really* fixed.

Clearing flags on attachment: 239053

Committed r174182: <http://trac.webkit.org/changeset/174182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 137334

[Daniel Bates]

(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).