Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1149743002:
[Findit] Use step level analysis to exclude flaky test failures. (Closed)
Description[Findit] Use step level analysis to exclude flaky test failures.
1. Try to retrieve archived step log in google cloud storage before analyze stdio log.
2. Pull reliable failures to a new log to extract signals.
3. Ignore flaky tests: If one test fails in some attempts and succeeds in others, it's a flaky test.
BUG=485795
Committed: https://chromium.googlesource.com/infra/infra/+/1ac9ac8d39c9d9a75c039bec10a09b67e8256c7a
Patch Set 1 #
Total comments: 9
Patch Set 2 : Use cStringIO to pull the reliable test failures. #
Total comments: 22
Patch Set 3 : Fixed several small issues based on comments. #
Total comments: 16
Patch Set 4 : Add check for 'gtest_results' being 'invalid'. #
Total comments: 16
Patch Set 5 : Add logic to handle cases when 'gtest_results' is 'invalid' or flaky. #
Total comments: 13
Patch Set 6 : Fix nits. #
Total comments: 1
Patch Set 7 : Fix name style nit. #Messages
Total messages: 34 (4 generated)
chanli@chromium.org changed reviewers: + katesonia@chromium.org, stgao@chromium.org
Hi, This is the change for step level analysis. Please review it when you have time. ^_^ Thx.
Publish some comments to work on. Technically speaking, this CL is to ignore the output of flaky tests. It is still not test-level analysis which includes detecting first-failure/last-pass of a test, etc. Sharu, please also review this CL to get an experience :) https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/buildbot.py:9: import gzip not in order. https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/buildbot.py:103: def CreateStepLogPath(master_name, builder_name, build_number, step_name): "GtestResult" might be more clear than "StepLog". Same for others below. https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/buildbot.py:145: step_log_file = gcs.open(CreateStepLogPath( Let us use "with" for the two opens here. It is cleaner. Please refer to contextlib.closing in https://chromium.googlesource.com/infra/infra/+/master/appengine/sheriff_o_ma... https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/extract_signal_pipeline.py (left): https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/extract_signal_pipeline.py:87: # TODO: Use archived stdio logs in Google Storage instead. Please still keep this TODO. Because we still pull logs from the build master if gtest result is not available. https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/extract_signal_pipeline.py:81: if test['status'] != 'FAILURE': This code doesn't match the comment. I think status has more values, like TIMEOUT, CRASHED, etc. You may check https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ru... https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/extract_signal_pipeline.py:113: if not lock_util.WaitUntilDownloadAllowed( We don't have to wait unless the request is to the build master. https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/extract_signal_pipeline.py:118: # TODO: Add test level log info to signal style. https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/extract_signal_pipeline.py:122: test_failure_log = str(self._GetTestLevelFailures(step_log)) This is a little hacky. How about using StringIO in self._GetTestLevelFailures to build it? https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/test/buildbot_test.py:11: import gzip order.
I've made the changes, please review it when you have time.^_^
Gave a more closer review. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:144: """Returns the archived Step Log file.""" This function is to return the content of the gtest json results for the gtest-based step. So technically it is not step log file. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:210: gtest_results = step_data.get('logs') This is actually 'step_logs'. Let's keep it as it was. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:51: def _GetTestLevelFailures(gtest_result): Maybe rename it to _GetReliableTestFailureLog? or FailureOutput? https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:52: """Analyze the step log and extract reliable failures only. not step log. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:55: gtest_result (file): A JSON file for failed step log. This is not a file, it is a string in JSON format. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:60: no empty line here. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:64: for key in iteration.keys(): # Keys are test names. key -> test_name That makes the code more clear. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:73: if is_reliable_failure: # All attempts failed, it's a reliable failure. comment style. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:103: test_failure_log = step.log_data failure_log? Because it could be compile step too, not just test step. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:123: # again in next run, because that might lead to DDoS to the master. Please keep the original TODO here. See my last comment. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:146: str(test_failure_log)).ToDict() Why we need str here? Same reason as you told me? https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:7: import unittest Unused now? https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:8: from testing_utils import testing Why APIs from unittest are not enough? https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:11: import gzip style: order and category still not fixed. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:117: expected_failure_log = ("'Unittest2.Subtest1': a/b/u2s1.cc:567: Failure\\n"+ No double quote in python code. Escape \' if needed. And no + is needed. long string could be like this: var_name = ( '1line' '2line' '...' )
Have some questions and find a nit :) https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:127: raise pipeline.Retry('Failed to pull stdio of step %s of master %s' I think if we fail to get result from self._GetTestLevelFailures, test_failure_log may also be empty, should we check that? https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:158: 'a/b/u2s1.cc': [567], Should the indent here be 4 spaces?
Done. Please review it when you have time. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:52: """Analyze the step log and extract reliable failures only. On 2015/05/22 01:30:37, Shuotao wrote: > not step log. I think it actually is the 'archived' step log. isn't it? https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:127: raise pipeline.Retry('Failed to pull stdio of step %s of master %s' On 2015/05/22 17:54:30, sharu jiang wrote: > I think if we fail to get result from self._GetTestLevelFailures, > test_failure_log may also be empty, should we check that? We will go to stdio log if we failed retrieving data from archived step log. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:146: str(test_failure_log)).ToDict() On 2015/05/22 01:30:37, Shuotao wrote: > Why we need str here? Same reason as you told me? No... I forgot this one. Fixed. https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:8: from testing_utils import testing On 2015/05/22 01:30:37, Shuotao wrote: > Why APIs from unittest are not enough? I don't quite remember, but I think back then I need to change it to testing.AppengineTestCase because some change I made. I think I might have removed those changes.
some follow ups :) https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:127: raise pipeline.Retry('Failed to pull stdio of step %s of master %s' On 2015/05/22 18:43:27, chanli wrote: > On 2015/05/22 17:54:30, sharu jiang wrote: > > I think if we fail to get result from self._GetTestLevelFailures, > > test_failure_log may also be empty, should we check that? > > We will go to stdio log if we failed retrieving data from archived step log. I mean even though we have gtest_result, we may not get any test_failure_log, for example if all the tests are unreliable
On 2015/05/22 20:09:37, sharu jiang wrote: > some follow ups :) > > https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... > File appengine/findit/waterfall/extract_signal_pipeline.py (right): > > https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... > appengine/findit/waterfall/extract_signal_pipeline.py:127: raise > pipeline.Retry('Failed to pull stdio of step %s of master %s' > On 2015/05/22 18:43:27, chanli wrote: > > On 2015/05/22 17:54:30, sharu jiang wrote: > > > I think if we fail to get result from self._GetTestLevelFailures, > > > test_failure_log may also be empty, should we check that? > > > > We will go to stdio log if we failed retrieving data from archived step log. > > I mean even though we have gtest_result, we may not get any test_failure_log, > for example if all the tests are unreliable I think in that case, we just don't have reliable failures, which means the whole step failure should be flaky. Thus we should not provide any suspected cls.
Get it but should we set it to some value, so the next time we when we check from WfStep.get(), we don't need to do all the processes again? On Fri, May 22, 2015 at 1:58 PM, <chanli@chromium.org> wrote: > On 2015/05/22 20:09:37, sharu jiang wrote: > >> some follow ups :) >> > > > > https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... > >> File appengine/findit/waterfall/extract_signal_pipeline.py (right): >> > > > > https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... > >> appengine/findit/waterfall/extract_signal_pipeline.py:127: raise >> pipeline.Retry('Failed to pull stdio of step %s of master %s' >> On 2015/05/22 18:43:27, chanli wrote: >> > On 2015/05/22 17:54:30, sharu jiang wrote: >> > > I think if we fail to get result from self._GetTestLevelFailures, >> > > test_failure_log may also be empty, should we check that? >> > >> > We will go to stdio log if we failed retrieving data from archived step >> log. >> > > > I mean even though we have gtest_result, we may not get any >> test_failure_log, >> for example if all the tests are unreliable >> > > I think in that case, we just don't have reliable failures, which means the > whole step failure should be flaky. Thus we should not provide any > suspected > cls. > > https://codereview.chromium.org/1149743002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
So maybe we should set the failure_log to "No reliable failures found." ? What do you tow think? On Fri, May 22, 2015 at 2:38 PM, Sharu Jiang <katesonia@google.com> wrote: > Get it > > but should we set it to some value, so the next time we when we check from > WfStep.get(), we don't need to do all the processes again? > > On Fri, May 22, 2015 at 1:58 PM, <chanli@chromium.org> wrote: > >> On 2015/05/22 20:09:37, sharu jiang wrote: >> >>> some follow ups :) >>> >> >> >> >> https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... >> >>> File appengine/findit/waterfall/extract_signal_pipeline.py (right): >>> >> >> >> >> https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... >> >>> appengine/findit/waterfall/extract_signal_pipeline.py:127: raise >>> pipeline.Retry('Failed to pull stdio of step %s of master %s' >>> On 2015/05/22 18:43:27, chanli wrote: >>> > On 2015/05/22 17:54:30, sharu jiang wrote: >>> > > I think if we fail to get result from self._GetTestLevelFailures, >>> > > test_failure_log may also be empty, should we check that? >>> > >>> > We will go to stdio log if we failed retrieving data from archived >>> step log. >>> >> >> >> I mean even though we have gtest_result, we may not get any >>> test_failure_log, >>> for example if all the tests are unreliable >>> >> >> I think in that case, we just don't have reliable failures, which means >> the >> whole step failure should be flaky. Thus we should not provide any >> suspected >> cls. >> >> https://codereview.chromium.org/1149743002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Maybe, 'failures are flaky?' I am not sure On Fri, May 22, 2015 at 3:20 PM, Chan Li <chanli@google.com> wrote: > So maybe we should set the failure_log to "No reliable failures found." ? > What do you tow think? > > On Fri, May 22, 2015 at 2:38 PM, Sharu Jiang <katesonia@google.com> wrote: > >> Get it >> >> but should we set it to some value, so the next time we when we check >> from WfStep.get(), we don't need to do all the processes again? >> >> On Fri, May 22, 2015 at 1:58 PM, <chanli@chromium.org> wrote: >> >>> On 2015/05/22 20:09:37, sharu jiang wrote: >>> >>>> some follow ups :) >>>> >>> >>> >>> >>> https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... >>> >>>> File appengine/findit/waterfall/extract_signal_pipeline.py (right): >>>> >>> >>> >>> >>> https://codereview.chromium.org/1149743002/diff/20001/appengine/findit/waterf... >>> >>>> appengine/findit/waterfall/extract_signal_pipeline.py:127: raise >>>> pipeline.Retry('Failed to pull stdio of step %s of master %s' >>>> On 2015/05/22 18:43:27, chanli wrote: >>>> > On 2015/05/22 17:54:30, sharu jiang wrote: >>>> > > I think if we fail to get result from self._GetTestLevelFailures, >>>> > > test_failure_log may also be empty, should we check that? >>>> > >>>> > We will go to stdio log if we failed retrieving data from archived >>>> step log. >>>> >>> >>> >>> I mean even though we have gtest_result, we may not get any >>>> test_failure_log, >>>> for example if all the tests are unreliable >>>> >>> >>> I think in that case, we just don't have reliable failures, which means >>> the >>> whole step failure should be flaky. Thus we should not provide any >>> suspected >>> cls. >>> >>> https://codereview.chromium.org/1149743002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Overall looks quite good. For python coding style, please check http://google-styleguide.googlecode.com/svn/trunk/pyguide.html https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:143: builder_name, build_number, step_name): # pragma: no cover style: parameter indent. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:52: """Analyze the archived step log and extract reliable failures only. To be accurate, it is 'archived gtest json results'. For 'archived' step log, it is the whole stdio log of a step. Gtest json results is a subset of it and in json format. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:58: A string contains the names of reliable test failures and output_snippets. 'output_snippets' is a detail about the gtest output. So it's better to hide detail from the user of this function. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:66: for test in iteration[test_name]: test->test_run? https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:72: if is_reliable_failure: # all attempts failed, it's a reliable failure Still not correct, please check the google python style. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:107: if gtest_result: https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ru... We need to fall back to stdio log if the gtest result is invalid. You may think about where the check should be added. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:7: import cloudstorage as gcs This is a third-party lib, so it has to be in a separate category. Please check google python style. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:6: import json style: order
https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:143: builder_name, build_number, step_name): # pragma: no cover On 2015/05/22 22:49:42, Shuotao wrote: > style: parameter indent. Done. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:52: """Analyze the archived step log and extract reliable failures only. On 2015/05/22 22:49:43, Shuotao wrote: > To be accurate, it is 'archived gtest json results'. > > For 'archived' step log, it is the whole stdio log of a step. Gtest json results > is a subset of it and in json format. Done. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:58: A string contains the names of reliable test failures and output_snippets. On 2015/05/22 22:49:43, Shuotao wrote: > 'output_snippets' is a detail about the gtest output. So it's better to hide > detail from the user of this function. Done. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:66: for test in iteration[test_name]: On 2015/05/22 22:49:42, Shuotao wrote: > test->test_run? Done. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:72: if is_reliable_failure: # all attempts failed, it's a reliable failure On 2015/05/22 22:49:43, Shuotao wrote: > Still not correct, please check the google python style. Done. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:107: if gtest_result: On 2015/05/22 22:49:43, Shuotao wrote: > https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ru... > > We need to fall back to stdio log if the gtest result is invalid. > You may think about where the check should be added. Done. https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:7: import cloudstorage as gcs On 2015/05/22 22:49:43, Shuotao wrote: > This is a third-party lib, so it has to be in a separate category. Please check > google python style. Done. One question: I don't see this in the https://google-styleguide.googlecode.com/svn/trunk/pyguide.html Do you mind to point it ot to me? https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/40001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:6: import json On 2015/05/22 22:49:43, Shuotao wrote: > style: order json is actually not used. Done.
https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:149: with contextlib.closing(gzip.open( Is contextlib.closing required for gzip.open? https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:62: sio = cStringIO.StringIO() This could be moved to after the check of 'invalid'. https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:113: failure_log = self._GetReliableTestFailureLog(gtest_result) If the gtest_result is 'invalid', we should fallback to stdio instead. However, the code here seems not like that. https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:7: import cloudstorage as gcs https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Imports_forma... https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:146: def _MockGetGtestResultLog(master_name, '_' could be removed. Because function defined here is not visible in the module.
Hi Shuotao, I have 2 questions about your comments... https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:149: with contextlib.closing(gzip.open( On 2015/05/26 18:14:49, Shuotao wrote: > Is contextlib.closing required for gzip.open? Honestly I'm not sure about this part of code since I don't know how to test it. https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:62: sio = cStringIO.StringIO() On 2015/05/26 18:14:49, Shuotao wrote: > This could be moved to after the check of 'invalid'. Done. https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:113: failure_log = self._GetReliableTestFailureLog(gtest_result) On 2015/05/26 18:14:49, Shuotao wrote: > If the gtest_result is 'invalid', we should fallback to stdio instead. > However, the code here seems not like that. Now we will both get None as return if we get 'invalid' as gtest_result, or if all the failures are flaky. I'm thinking maybe I can return different messages and only fallback to stdio if we get 'invalid'? https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:7: import cloudstorage as gcs On 2015/05/26 18:14:49, Shuotao wrote: > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Imports_forma... Done. https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:146: def _MockGetGtestResultLog(master_name, On 2015/05/26 18:14:49, Shuotao wrote: > '_' could be removed. > Because function defined here is not visible in the module. Done.
https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:149: with contextlib.closing(gzip.open( On 2015/05/26 19:07:38, chanli wrote: > On 2015/05/26 18:14:49, Shuotao wrote: > > Is contextlib.closing required for gzip.open? > > Honestly I'm not sure about this part of code since I don't know how to test it. You may download a gtest-result .json.gz file and use a local script to test it out :) https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:113: failure_log = self._GetReliableTestFailureLog(gtest_result) On 2015/05/26 19:07:38, chanli wrote: > On 2015/05/26 18:14:49, Shuotao wrote: > > If the gtest_result is 'invalid', we should fallback to stdio instead. > > However, the code here seems not like that. > > Now we will both get None as return if we get 'invalid' as gtest_result, or if > all the failures are flaky. > > I'm thinking maybe I can return different messages and only fallback to stdio if > we get 'invalid'? Sounds good to me.
https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:149: with contextlib.closing(gzip.open( On 2015/05/26 19:17:11, Shuotao wrote: > On 2015/05/26 19:07:38, chanli wrote: > > On 2015/05/26 18:14:49, Shuotao wrote: > > > Is contextlib.closing required for gzip.open? > > > > Honestly I'm not sure about this part of code since I don't know how to test > it. > > You may download a gtest-result .json.gz file and use a local script to test it > out :) But this will only test gzip but not gcs, right? Any suggestion about how I can test the function of gcs?
https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:149: with contextlib.closing(gzip.open( On 2015/05/26 18:14:49, Shuotao wrote: > Is contextlib.closing required for gzip.open? I think that with contextlib.closing I don't need to close gzip because contextlib has done that for me. But I just tested it even if I don't write gzip.close, the second time I use f.read() it will only return ''. Is this means I don't necessarily need to close gzip?
Done. https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:149: with contextlib.closing(gzip.open( On 2015/05/26 18:14:49, Shuotao wrote: > Is contextlib.closing required for gzip.open? Done. https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/60001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:113: failure_log = self._GetReliableTestFailureLog(gtest_result) On 2015/05/26 19:17:11, Shuotao wrote: > On 2015/05/26 19:07:38, chanli wrote: > > On 2015/05/26 18:14:49, Shuotao wrote: > > > If the gtest_result is 'invalid', we should fallback to stdio instead. > > > However, the code here seems not like that. > > > > Now we will both get None as return if we get 'invalid' as gtest_result, or if > > all the failures are flaky. > > > > I'm thinking maybe I can return different messages and only fallback to stdio > if > > we get 'invalid'? > > Sounds good to me. Done.
https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:148: build_number, step_name))) as gtest_result_file: Readability could be improved here by adding a new VAR for the gtest result path, and then use it in gcs.open. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:59: log content. We may want to update the docstring for the special returned value 'invalid' and 'flaky'. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:84: if failed_test_log == '': style nit: we could do below instead if not VAR: return 'something' # executed if VAR is None, '', or 0, etc. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:154: failure_log).ToDict() Can't this fit in one line? https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:10: import unittest unittest is in the standard python library, not a third-party library like gcs. is gcs used in this module? https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:139: build_number = 124 I didn't see the data file for build 124 and 125. Forgot to upload for review? https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:175: # Mock both stdiolog and step log to test whether Findit will nit: not "step log", it is "gtest json results". Same for function or variable names in the code.
Done. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/buildbot.py:148: build_number, step_name))) as gtest_result_file: On 2015/05/27 00:18:21, Shuotao wrote: > Readability could be improved here by adding a new VAR for the gtest result > path, and then use it in gcs.open. Done. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/extract_signal_pipeline.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:84: if failed_test_log == '': On 2015/05/27 00:18:21, Shuotao wrote: > style nit: we could do below instead > > if not VAR: > return 'something' # executed if VAR is None, '', or 0, etc. Done. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/extract_signal_pipeline.py:154: failure_log).ToDict() On 2015/05/27 00:18:21, Shuotao wrote: > Can't this fit in one line? Done. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/test/buildbot_test.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/test/buildbot_test.py:10: import unittest On 2015/05/27 00:18:21, Shuotao wrote: > unittest is in the standard python library, not a third-party library like gcs. > > is gcs used in this module? Done. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... File appengine/findit/waterfall/test/extract_signal_pipeline_test.py (right): https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:139: build_number = 124 On 2015/05/27 00:18:21, Shuotao wrote: > I didn't see the data file for build 124 and 125. Forgot to upload for review? Done. https://codereview.chromium.org/1149743002/diff/80001/appengine/findit/waterf... appengine/findit/waterfall/test/extract_signal_pipeline_test.py:175: # Mock both stdiolog and step log to test whether Findit will On 2015/05/27 00:18:21, Shuotao wrote: > nit: not "step log", it is "gtest json results". > > Same for function or variable names in the code. Done.
lgtm Would you like uploading a version to findit-for-me app and trying it out with https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... This build failure had test-level gtest json results. Thanks, Shuotao Gao https://codereview.chromium.org/1149743002/diff/100001/appengine/findit/water... File appengine/findit/waterfall/buildbot.py (right): https://codereview.chromium.org/1149743002/diff/100001/appengine/findit/water... appengine/findit/waterfall/buildbot.py:146: archivedLogPath = CreateGtestResultPath( style nit: naming_style.
The CQ bit was checked by chanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org Link to the patchset: https://codereview.chromium.org/1149743002/#ps120001 (title: "Fix name style nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149743002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/infra/infra/+/1ac9ac8d39c9d9a75c039bec10a09...
Message was sent while issue was closed.
Shuotao, I just modified buildbot.py and now gzip works. Could you give me another round of review?
Message was sent while issue was closed.
On 2015/05/27 23:33:56, chanli wrote: > Shuotao, > > I just modified buildbot.py and now gzip works. Could you give me another round > of review? Would you mind creating a new CL for the change as the original CL was already committed?
Message was sent while issue was closed.
On 2015/05/27 23:35:54, Shuotao wrote: > On 2015/05/27 23:33:56, chanli wrote: > > Shuotao, > > > > I just modified buildbot.py and now gzip works. Could you give me another > round > > of review? > > Would you mind creating a new CL for the change as the original CL was already > committed? OK, I can do that. Do I need to do anything about this new patch I just added? Or I can just leave it here?
Message was sent while issue was closed.
On 2015/05/27 23:37:04, chanli wrote: > On 2015/05/27 23:35:54, Shuotao wrote: > > On 2015/05/27 23:33:56, chanli wrote: > > > Shuotao, > > > > > > I just modified buildbot.py and now gzip works. Could you give me another > > round > > > of review? > > > > Would you mind creating a new CL for the change as the original CL was already > > committed? > > OK, I can do that. Do I need to do anything about this new patch I just added? > Or I can just leave it here? It might be better to delete it.
Message was sent while issue was closed.
How to delete it? On Wed, May 27, 2015 at 4:43 PM, <stgao@chromium.org> wrote: > On 2015/05/27 23:37:04, chanli wrote: > >> On 2015/05/27 23:35:54, Shuotao wrote: >> > On 2015/05/27 23:33:56, chanli wrote: >> > > Shuotao, >> > > >> > > I just modified buildbot.py and now gzip works. Could you give me >> another >> > round >> > > of review? >> > >> > Would you mind creating a new CL for the change as the original CL was >> > already > >> > committed? >> > > OK, I can do that. Do I need to do anything about this new patch I just >> added? >> Or I can just leave it here? >> > > It might be better to delete it. > > https://codereview.chromium.org/1149743002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Patchset #8 (id:140001) has been deleted
Message was sent while issue was closed.
Found it. Done. On Wed, May 27, 2015 at 4:44 PM, Chan Li <chanli@google.com> wrote: > How to delete it? > > On Wed, May 27, 2015 at 4:43 PM, <stgao@chromium.org> wrote: > >> On 2015/05/27 23:37:04, chanli wrote: >> >>> On 2015/05/27 23:35:54, Shuotao wrote: >>> > On 2015/05/27 23:33:56, chanli wrote: >>> > > Shuotao, >>> > > >>> > > I just modified buildbot.py and now gzip works. Could you give me >>> another >>> > round >>> > > of review? >>> > >>> > Would you mind creating a new CL for the change as the original CL was >>> >> already >> >>> > committed? >>> >> >> OK, I can do that. Do I need to do anything about this new patch I just >>> added? >>> Or I can just leave it here? >>> >> >> It might be better to delete it. >> >> https://codereview.chromium.org/1149743002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
