Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
850013004:
Obtain confidence score based off last known good and first known bad revision results. (Closed)
DescriptionObtain confidence score based off last known good and first known bad revision results.
BUG=448817
Committed: https://crrev.com/bdfab875b72358b19ef3b8434c6527f1192a506b
Cr-Commit-Position: refs/heads/master@{#314257}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressing feedback #
Messages
Total messages: 11 (3 generated)
robertocn@chromium.org changed reviewers: + prasadv@chromium.org, qyearsley@chromium.org
Please review. There are a couple minor typos in comments and docstrings that I am already aware of, I added comments on them. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... File tools/auto_bisect/bisect_perf_regression.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:2445: # Get new confidence score This comment will be removed. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... File tools/auto_bisect/bisect_results.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:235: Note that since revision_states is expected to be in reverse cronological chronological not cronological. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:238: `first_working_revision`. The inverse applies to `last_broken_revision`.\ Unnecessary backlash at the end of the line.(typo) https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... File tools/auto_bisect/bisect_results_test.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results_test.py:230: revision_states[2].value = {'values': [95, 90, 90]} Because of the new way of obtaining the confidence, needed to alter this test so it would yield a confidence greater than 0, but yielding a warning.
Ping!
https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... File tools/auto_bisect/bisect_perf_regression.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:1254: and the timeout period specified in self.opts. Possible suggestion: Rename this to "repeat_count_multiplier" (or "test_run_multiplier"), give it a default value of 1 in the args of RunTest, and change below to: for i in xrange(self.opts.repeat_test_count * repeat_count_multiplier): ... And change the time limit in the same way. Would this be clearer or no? https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:2432: if(bool(good_state.passed) != bool(bad_state.passed) Add space after "if" https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:2442: ) Since this is a function call, the convention is to put the closing parenthesis on the same line as the last arg. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... File tools/auto_bisect/bisect_results.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:232: def FindBreakingRevRange(revision_states): I think it's probably worth it to add a test for this function, even though it's probably at least covered by the other tests methods. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:239: """ [Optional] You could add Args and Returns sections to this docstring which explicitly says that the input is a list of RevisionState and the output is a pair of RevisionState. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:241: last_broken_revision = None [Optional] It might be less confusing to reverse the list so that it's in chronological order, and call these last_good_rev and first_bad_rev. This could be done in a separate CL, changing the other usages as well, and making the revision states list chronological order.
qyearsley@chromium.org changed reviewers: + chrisphan@chromium.org
Addressed all points. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... File tools/auto_bisect/bisect_perf_regression.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:1254: and the timeout period specified in self.opts. On 2015/01/16 18:37:03, qyearsley wrote: > Possible suggestion: > Rename this to "repeat_count_multiplier" (or "test_run_multiplier"), give it a > default value of 1 in the args of RunTest, and change below to: > for i in xrange(self.opts.repeat_test_count * repeat_count_multiplier): > ... > And change the time limit in the same way. > > Would this be clearer or no? Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:2432: if(bool(good_state.passed) != bool(bad_state.passed) On 2015/01/16 18:37:03, qyearsley wrote: > Add space after "if" Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:2442: ) On 2015/01/16 18:37:03, qyearsley wrote: > Since this is a function call, the convention is to put the closing parenthesis > on the same line as the last arg. Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... tools/auto_bisect/bisect_perf_regression.py:2445: # Get new confidence score On 2015/01/14 18:50:47, robertocn wrote: > This comment will be removed. Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... File tools/auto_bisect/bisect_results.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:232: def FindBreakingRevRange(revision_states): On 2015/01/16 18:37:03, qyearsley wrote: > I think it's probably worth it to add a test for this function, even though it's > probably at least covered by the other tests methods. Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:235: Note that since revision_states is expected to be in reverse cronological On 2015/01/14 18:50:47, robertocn wrote: > chronological not cronological. Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:238: `first_working_revision`. The inverse applies to `last_broken_revision`.\ On 2015/01/14 18:50:47, robertocn wrote: > Unnecessary backlash at the end of the line.(typo) Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:239: """ On 2015/01/16 18:37:03, qyearsley wrote: > [Optional] You could add Args and Returns sections to this docstring which > explicitly says that the input is a list of RevisionState and the output is a > pair of RevisionState. Done. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results.py:241: last_broken_revision = None On 2015/01/16 18:37:03, qyearsley wrote: > [Optional] It might be less confusing to reverse the list so that it's in > chronological order, and call these last_good_rev and first_bad_rev. This could > be done in a separate CL, changing the other usages as well, and making the > revision states list chronological order. Acknowledged. https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... File tools/auto_bisect/bisect_results_test.py (right): https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... tools/auto_bisect/bisect_results_test.py:230: revision_states[2].value = {'values': [95, 90, 90]} On 2015/01/14 18:50:47, robertocn wrote: > Because of the new way of obtaining the confidence, needed to alter this test so > it would yield a confidence greater than 0, but yielding a warning. Done.
On 2015/01/30 21:23:32, robertocn wrote: > Addressed all points. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... > File tools/auto_bisect/bisect_perf_regression.py (right): > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... > tools/auto_bisect/bisect_perf_regression.py:1254: and the timeout period > specified in self.opts. > On 2015/01/16 18:37:03, qyearsley wrote: > > Possible suggestion: > > Rename this to "repeat_count_multiplier" (or "test_run_multiplier"), give it a > > default value of 1 in the args of RunTest, and change below to: > > for i in xrange(self.opts.repeat_test_count * repeat_count_multiplier): > > ... > > And change the time limit in the same way. > > > > Would this be clearer or no? > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... > tools/auto_bisect/bisect_perf_regression.py:2432: if(bool(good_state.passed) != > bool(bad_state.passed) > On 2015/01/16 18:37:03, qyearsley wrote: > > Add space after "if" > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... > tools/auto_bisect/bisect_perf_regression.py:2442: ) > On 2015/01/16 18:37:03, qyearsley wrote: > > Since this is a function call, the convention is to put the closing > parenthesis > > on the same line as the last arg. > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_per... > tools/auto_bisect/bisect_perf_regression.py:2445: # Get new confidence score > On 2015/01/14 18:50:47, robertocn wrote: > > This comment will be removed. > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > File tools/auto_bisect/bisect_results.py (right): > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > tools/auto_bisect/bisect_results.py:232: def > FindBreakingRevRange(revision_states): > On 2015/01/16 18:37:03, qyearsley wrote: > > I think it's probably worth it to add a test for this function, even though > it's > > probably at least covered by the other tests methods. > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > tools/auto_bisect/bisect_results.py:235: Note that since revision_states is > expected to be in reverse cronological > On 2015/01/14 18:50:47, robertocn wrote: > > chronological not cronological. > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > tools/auto_bisect/bisect_results.py:238: `first_working_revision`. The inverse > applies to `last_broken_revision`.\ > On 2015/01/14 18:50:47, robertocn wrote: > > Unnecessary backlash at the end of the line.(typo) > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > tools/auto_bisect/bisect_results.py:239: """ > On 2015/01/16 18:37:03, qyearsley wrote: > > [Optional] You could add Args and Returns sections to this docstring which > > explicitly says that the input is a list of RevisionState and the output is a > > pair of RevisionState. > > Done. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > tools/auto_bisect/bisect_results.py:241: last_broken_revision = None > On 2015/01/16 18:37:03, qyearsley wrote: > > [Optional] It might be less confusing to reverse the list so that it's in > > chronological order, and call these last_good_rev and first_bad_rev. This > could > > be done in a separate CL, changing the other usages as well, and making the > > revision states list chronological order. > > Acknowledged. > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > File tools/auto_bisect/bisect_results_test.py (right): > > https://codereview.chromium.org/850013004/diff/1/tools/auto_bisect/bisect_res... > tools/auto_bisect/bisect_results_test.py:230: revision_states[2].value = > {'values': [95, 90, 90]} > On 2015/01/14 18:50:47, robertocn wrote: > > Because of the new way of obtaining the confidence, needed to alter this test > so > > it would yield a confidence greater than 0, but yielding a warning. > > Done. Awesome, lgtm.
The CQ bit was checked by robertocn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/850013004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bdfab875b72358b19ef3b8434c6527f1192a506b Cr-Commit-Position: refs/heads/master@{#314257} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
