RESOLVED FIXED 137303
commitqueuetasks_unittest references a non-existent member variable when mock-reporting flakey tests.
https://bugs.webkit.org/show_bug.cgi?id=137303
Summary commitqueuetasks_unittest references a non-existent member variable when mock...
Jake Nielsen
Reported 2014-10-01 10:08:01 PDT
MockCommitQueue tries to access the "filename" member of a TestResult in unexcercised code.
Attachments
Makes the appropriate name change (2.19 KB, patch)
2014-10-01 10:18 PDT, Jake Nielsen
dbates: review-
dbates: commit-queue-
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-
typo fixed (4.62 KB, patch)
2014-10-01 14:58 PDT, Jake Nielsen
no flags
typo *really* fixed. (4.62 KB, patch)
2014-10-01 15:02 PDT, Jake Nielsen
no flags
Jake Nielsen
Comment 1 2014-10-01 10:18:52 PDT
Created attachment 239033 [details] Makes the appropriate name change
Daniel Bates
Comment 2 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.
Jake Nielsen
Comment 3 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.
Daniel Bates
Comment 4 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)
Daniel Bates
Comment 5 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...
Jake Nielsen
Comment 6 2014-10-01 14:01:59 PDT
Created attachment 239048 [details] Adds a unit test to exercise the mock
Daniel Bates
Comment 7 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".
Jake Nielsen
Comment 8 2014-10-01 14:58:00 PDT
Created attachment 239051 [details] typo fixed
Jake Nielsen
Comment 9 2014-10-01 14:58:50 PDT
Hold up, I uploaded the wrong patch.
Jake Nielsen
Comment 10 2014-10-01 15:02:49 PDT
Created attachment 239053 [details] typo *really* fixed.
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2014-10-01 15:46:47 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 13 2014-10-01 23:17:07 PDT
Re-opened since this is blocked by bug 137334
Daniel Bates
Comment 14 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).
Note You need to log in before you can comment on or make changes to this bug.