WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug