MockCommitQueue tries to access the "filename" member of a TestResult in unexcercised code.
Created attachment 239033 [details] Makes the appropriate name change
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.
(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.
(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)
(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...
Created attachment 239048 [details] Adds a unit test to exercise the mock
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".
Created attachment 239051 [details] typo fixed
Hold up, I uploaded the wrong patch.
Created attachment 239053 [details] typo *really* fixed.
Comment on attachment 239053 [details] typo *really* fixed. Clearing flags on attachment: 239053 Committed r174182: <http://trac.webkit.org/changeset/174182>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 137334
(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).