Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
411143003:
Add an "in_unit_test_mode" attribute to the environment. Use this to default output format to 'gtes… (Closed)
|
Created:
6 years, 5 months ago by chrishenry Modified:
6 years, 4 months ago CC:
chromium-reviews, piman+watch_chromium.org, telemetry+watch_chromium.org, jam, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd an "in_unit_test_mode" attribute to the environment. Use this to default output format to 'gtest' for unit test.
Previously, we default output format to 'gtest' if the PageTest object
being run is not a PageMeasurement. This distinction between PageTest
and PageMeasurement is going away though. The new mechanism is also
more explicit.
BUG=383635
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address review comments. #
Total comments: 1
Patch Set 3 : Address review comment. #
Total comments: 1
Patch Set 4 : New approach (a wrapper script). #
Messages
Total messages: 22 (0 generated)
LGTM https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/environment.py (right): https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/environment.py:24: def in_unit_test_mode(self): Is it worth documenting the precise meaning of this property? https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... File tools/telemetry/telemetry/test_runner.py (right): https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... tools/telemetry/telemetry/test_runner.py:350: options.output_format = 'gtest' Is it worth allowing overriding of this -- for example, if the output_format is not specified?
https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/environment.py (right): https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/environment.py:24: def in_unit_test_mode(self): On 2014/07/23 23:46:26, Ken Russell wrote: > Is it worth documenting the precise meaning of this property? Done. https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... File tools/telemetry/telemetry/test_runner.py (right): https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... tools/telemetry/telemetry/test_runner.py:350: options.output_format = 'gtest' On 2014/07/23 23:46:26, Ken Russell wrote: > Is it worth allowing overriding of this -- for example, if the output_format is > not specified? Will you have a use case for that? This currently is the equivalent of what we're doing in results_options.py, which is only used by gpu test (afaict). Personally, I don't think it is useful. We should distinguish between test and benchmark. output_format is useful in benchmark scenario.
https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... File tools/telemetry/telemetry/test_runner.py (right): https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... tools/telemetry/telemetry/test_runner.py:350: options.output_format = 'gtest' On 2014/07/23 23:52:59, chrishenry wrote: > On 2014/07/23 23:46:26, Ken Russell wrote: > > Is it worth allowing overriding of this -- for example, if the output_format > is > > not specified? > > Will you have a use case for that? This currently is the equivalent of what > we're doing in results_options.py, which is only used by gpu test (afaict). > Personally, I don't think it is useful. We should distinguish between test and > benchmark. output_format is useful in benchmark scenario. I thought one medium-term plan was to stop emitting gtest style output, which must be parsed by downstream scripts, and start emitting JSON which can be uploaded to the various dashboards directly. https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/environment.py (right): https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/environment.py:29: telemetry.""" Close paren.
On 2014/07/23 23:57:17, Ken Russell wrote: > https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... > File tools/telemetry/telemetry/test_runner.py (right): > > https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... > tools/telemetry/telemetry/test_runner.py:350: options.output_format = 'gtest' > On 2014/07/23 23:52:59, chrishenry wrote: > > On 2014/07/23 23:46:26, Ken Russell wrote: > > > Is it worth allowing overriding of this -- for example, if the output_format > > is > > > not specified? > > > > Will you have a use case for that? This currently is the equivalent of what > > we're doing in results_options.py, which is only used by gpu test (afaict). > > Personally, I don't think it is useful. We should distinguish between test and > > benchmark. output_format is useful in benchmark scenario. > > I thought one medium-term plan was to stop emitting gtest style output, which > must be parsed by downstream scripts, and start emitting JSON which can be > uploaded to the various dashboards directly. > > https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/environment.py (right): > > https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/environment.py:29: telemetry.""" > Close paren. gtest like output is needed for streaming output to report the state of running page test, i.e: we are running this page, we have finished running this page, etc. One model is gtest output will always be used for streaming results to stdout, and all other format output form will be output to file, but we haven't reached consensus on this yet. Another way of removing the weird hack of default format to gtest if PageTestResult is used is explicitly specifying benchmarks in gpu_tests/ to use output_format = 'gtest'. I prefer this over adding the output format bit to Environment. not lgtm
On 2014/07/24 00:42:41, nednguyen wrote: > On 2014/07/23 23:57:17, Ken Russell wrote: > > > https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... > > File tools/telemetry/telemetry/test_runner.py (right): > > > > > https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... > > tools/telemetry/telemetry/test_runner.py:350: options.output_format = 'gtest' > > On 2014/07/23 23:52:59, chrishenry wrote: > > > On 2014/07/23 23:46:26, Ken Russell wrote: > > > > Is it worth allowing overriding of this -- for example, if the > output_format > > > is > > > > not specified? > > > > > > Will you have a use case for that? This currently is the equivalent of what > > > we're doing in results_options.py, which is only used by gpu test (afaict). > > > Personally, I don't think it is useful. We should distinguish between test > and > > > benchmark. output_format is useful in benchmark scenario. > > > > I thought one medium-term plan was to stop emitting gtest style output, which > > must be parsed by downstream scripts, and start emitting JSON which can be > > uploaded to the various dashboards directly. I don't think we're changing the format for unit tests. The plan is to do the above only for benchmark, IIUC. > > > > > https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... > > File tools/telemetry/telemetry/core/environment.py (right): > > > > > https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... > > tools/telemetry/telemetry/core/environment.py:29: telemetry.""" > > Close paren. > > gtest like output is needed for streaming output to report the state of running > page test, i.e: we are running this page, we have finished running this page, > etc. > One model is gtest output will always be used for streaming results to stdout, > and all other format output form will be output to file, but we haven't reached > consensus on this yet. > > > Another way of removing the weird hack of default format to gtest if > PageTestResult is used is explicitly specifying benchmarks in gpu_tests/ to use > output_format = 'gtest'. I prefer this over adding the output format bit to > Environment. not lgtm How would you do that? Add an output_format parameter to Environment? Something else?
On 2014/07/24 00:53:45, chrishenry wrote: > On 2014/07/24 00:42:41, nednguyen wrote: > > On 2014/07/23 23:57:17, Ken Russell wrote: > > > > > > https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... > > > File tools/telemetry/telemetry/test_runner.py (right): > > > > > > > > > https://codereview.chromium.org/411143003/diff/1/tools/telemetry/telemetry/te... > > > tools/telemetry/telemetry/test_runner.py:350: options.output_format = > 'gtest' > > > On 2014/07/23 23:52:59, chrishenry wrote: > > > > On 2014/07/23 23:46:26, Ken Russell wrote: > > > > > Is it worth allowing overriding of this -- for example, if the > > output_format > > > > is > > > > > not specified? > > > > > > > > Will you have a use case for that? This currently is the equivalent of > what > > > > we're doing in results_options.py, which is only used by gpu test > (afaict). > > > > Personally, I don't think it is useful. We should distinguish between test > > and > > > > benchmark. output_format is useful in benchmark scenario. > > > > > > I thought one medium-term plan was to stop emitting gtest style output, > which > > > must be parsed by downstream scripts, and start emitting JSON which can be > > > uploaded to the various dashboards directly. > > I don't think we're changing the format for unit tests. The plan is to do the > above only for benchmark, IIUC. > > > > > > > > > > https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... > > > File tools/telemetry/telemetry/core/environment.py (right): > > > > > > > > > https://codereview.chromium.org/411143003/diff/20001/tools/telemetry/telemetr... > > > tools/telemetry/telemetry/core/environment.py:29: telemetry.""" > > > Close paren. > > > > gtest like output is needed for streaming output to report the state of > running > > page test, i.e: we are running this page, we have finished running this page, > > etc. > > One model is gtest output will always be used for streaming results to stdout, > > and all other format output form will be output to file, but we haven't > reached > > consensus on this yet. > > > > > > Another way of removing the weird hack of default format to gtest if > > PageTestResult is used is explicitly specifying benchmarks in gpu_tests/ to > use > > output_format = 'gtest'. I prefer this over adding the output format bit to > > Environment. not lgtm > > How would you do that? Add an output_format parameter to Environment? Something > else? Discussed offline. Done. Ken, PTAL as well, since the change is now mostly on gpu tests code.
https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right): https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... content/test/gpu/gpu_tests/cloud_storage_test_base.py:50: options = {'output_format': 'gtest'} Where is it documented that a class variable named "options" is consulted later?
On 2014/07/24 01:24:34, Ken Russell wrote: > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right): > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > content/test/gpu/gpu_tests/cloud_storage_test_base.py:50: options = > {'output_format': 'gtest'} > Where is it documented that a class variable named "options" is consulted later? We sadly don't have enough documentation for these class variable. It's used as default argument for the benchmark here: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
On 2014/07/24 01:36:16, nednguyen wrote: > On 2014/07/24 01:24:34, Ken Russell wrote: > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right): > > > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > content/test/gpu/gpu_tests/cloud_storage_test_base.py:50: options = > > {'output_format': 'gtest'} > > Where is it documented that a class variable named "options" is consulted > later? > > We sadly don't have enough documentation for these class variable. It's used as > default argument for the benchmark here: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... I'm really not happy with the spaghetti-like nature of this fix. Am I allowed to ask that more thought be given? I don't know Telemetry's structure well enough to advise.
LGTM Please wait for Ken's feedback too.
On 2014/07/24 01:49:33, Ken Russell wrote: > On 2014/07/24 01:36:16, nednguyen wrote: > > On 2014/07/24 01:24:34, Ken Russell wrote: > > > > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > > File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right): > > > > > > > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > > content/test/gpu/gpu_tests/cloud_storage_test_base.py:50: options = > > > {'output_format': 'gtest'} > > > Where is it documented that a class variable named "options" is consulted > > later? > > > > We sadly don't have enough documentation for these class variable. It's used > as > > default argument for the benchmark here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > I'm really not happy with the spaghetti-like nature of this fix. Am I allowed to > ask that more thought be given? I don't know Telemetry's structure well enough > to advise. I don't like the wiring of options system either. But with removing the gtest as the default format, we need to specify your gpu_test to use the gtest-format somewhere. Setting class's variable options is one place, another place would be pass the flag ---output-format=gtest in your bot script. Either way, I am pretty sure that test environment is not the right place to specify that format.
On 2014/07/24 01:58:13, nednguyen wrote: > On 2014/07/24 01:49:33, Ken Russell wrote: > > On 2014/07/24 01:36:16, nednguyen wrote: > > > On 2014/07/24 01:24:34, Ken Russell wrote: > > > > > > > > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > > > File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > > > content/test/gpu/gpu_tests/cloud_storage_test_base.py:50: options = > > > > {'output_format': 'gtest'} > > > > Where is it documented that a class variable named "options" is consulted > > > later? > > > > > > We sadly don't have enough documentation for these class variable. It's used > > as > > > default argument for the benchmark here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > I'm really not happy with the spaghetti-like nature of this fix. Am I allowed > to > > ask that more thought be given? I don't know Telemetry's structure well enough > > to advise. > > > I don't like the wiring of options system either. But with removing the gtest as > the default format, we need to specify your gpu_test to use the gtest-format > somewhere. > Setting class's variable options is one place, another place would be pass the > flag ---output-format=gtest in your bot script. Actually, the GPU bots already specify --output-format=gtest when invoking all of these Telemetry-based correctness tests. See a recent build from http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20%28NVIDIA%29 , for example: http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20%28NVIDIA... I do appreciate the fact that this will make it easier to run these tests from the command line, so that we don't have to remember to specify --output-format=gtest all the time. I'd definitely forget that most of the time. > Either way, I am pretty sure that test environment is not the right place to > specify that format. What other choices are there? Could a proper API be added to page_test.PageTest for setting this option?
On 2014/07/24 02:08:05, Ken Russell wrote: > On 2014/07/24 01:58:13, nednguyen wrote: > > On 2014/07/24 01:49:33, Ken Russell wrote: > > > On 2014/07/24 01:36:16, nednguyen wrote: > > > > On 2014/07/24 01:24:34, Ken Russell wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > > > > File content/test/gpu/gpu_tests/cloud_storage_test_base.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/411143003/diff/40001/content/test/gpu/gpu_tes... > > > > > content/test/gpu/gpu_tests/cloud_storage_test_base.py:50: options = > > > > > {'output_format': 'gtest'} > > > > > Where is it documented that a class variable named "options" is > consulted > > > > later? > > > > > > > > We sadly don't have enough documentation for these class variable. It's > used > > > as > > > > default argument for the benchmark here: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > > > I'm really not happy with the spaghetti-like nature of this fix. Am I > allowed > > to > > > ask that more thought be given? I don't know Telemetry's structure well > enough > > > to advise. > > > > > > I don't like the wiring of options system either. But with removing the gtest > as > > the default format, we need to specify your gpu_test to use the gtest-format > > somewhere. > > Setting class's variable options is one place, another place would be pass the > > flag ---output-format=gtest in your bot script. > > Actually, the GPU bots already specify --output-format=gtest when invoking all > of these Telemetry-based correctness tests. See a recent build from > http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20%28NVIDIA%29 > , for example: > > http://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20%28NVIDIA... > > I do appreciate the fact that this will make it easier to run these tests from > the command line, so that we don't have to remember to specify > --output-format=gtest all the time. I'd definitely forget that most of the time. > Yes, instead of the syntactic sugar "options =..." , we can override SetArgumentDefaults to add gtest option. > > > Either way, I am pretty sure that test environment is not the right place to > > specify that format. > > What other choices are there? Could a proper API be added to page_test.PageTest > for setting this option? For this immediate patch, would it be fine for Chris to revert all the changes in gpu_tests/ and we do a subsequent patch to override SetArgumentDefaults(..) for the gpu_tests ?
On 2014/07/24 02:20:04, nednguyen wrote: > For this immediate patch, would it be fine for Chris to revert all the changes > in gpu_tests/ and we do a subsequent patch to override SetArgumentDefaults(..) > for the gpu_tests ? That sounds fine. Please manually submit try jobs to the GPU bots so we can make sure the tests' results are still being processed correctly before CQ'ing it. Thanks.
How about just a wrapper sh script that add --output_format=gtest, since bit already does the right thing? (: (from phone) On Jul 23, 2014 7:32 PM, <kbr@chromium.org> wrote: > On 2014/07/24 02:20:04, nednguyen wrote: > >> For this immediate patch, would it be fine for Chris to revert all the >> changes >> in gpu_tests/ and we do a subsequent patch to override >> SetArgumentDefaults(..) >> for the gpu_tests ? >> > > That sounds fine. Please manually submit try jobs to the GPU bots so we > can make > sure the tests' results are still being processed correctly before CQ'ing > it. > Thanks. > > > https://codereview.chromium.org/411143003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
(I meant bot.) (from phone) On Jul 23, 2014 7:44 PM, "Chris Henry" <chrishenry@google.com> wrote: > How about just a wrapper sh script that add --output_format=gtest, since > bit already does the right thing? (: > > (from phone) > On Jul 23, 2014 7:32 PM, <kbr@chromium.org> wrote: > >> On 2014/07/24 02:20:04, nednguyen wrote: >> >>> For this immediate patch, would it be fine for Chris to revert all the >>> changes >>> in gpu_tests/ and we do a subsequent patch to override >>> SetArgumentDefaults(..) >>> for the gpu_tests ? >>> >> >> That sounds fine. Please manually submit try jobs to the GPU bots so we >> can make >> sure the tests' results are still being processed correctly before CQ'ing >> it. >> Thanks. >> >> >> https://codereview.chromium.org/411143003/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, here is what I was thinking. Lmk what you think. Another option would be to just enable gtest output for the default output_format (html) as per conversation in mailing list. Then this script will not need to override output_format at all.
On 2014/07/24 15:44:46, chrishenry wrote: > Ok, here is what I was thinking. Lmk what you think. > > Another option would be to just enable gtest output for the default > output_format (html) as per conversation in mailing list. Then this script will > not need to override output_format at all. The wrapper script is confusing and won't work on Windows. We could just as easily override this option in run_gpu_test.py if it isn't specified on the command line. It would be preferable to land this CL without the wrapper script.
On 2014/07/25 23:40:35, Ken Russell wrote: > On 2014/07/24 15:44:46, chrishenry wrote: > > Ok, here is what I was thinking. Lmk what you think. > > > > Another option would be to just enable gtest output for the default > > output_format (html) as per conversation in mailing list. Then this script > will > > not need to override output_format at all. > > The wrapper script is confusing and won't work on Windows. We could just as > easily override this option in run_gpu_test.py if it isn't specified on the > command line. It would be preferable to land this CL without the wrapper script. In run_gpu_test.py: sys.exit(test_runner.main(sys.argv + ['--output-format', 'gtest']))
On 2014/07/29 22:28:51, dtu wrote: > On 2014/07/25 23:40:35, Ken Russell wrote: > > On 2014/07/24 15:44:46, chrishenry wrote: > > > Ok, here is what I was thinking. Lmk what you think. > > > > > > Another option would be to just enable gtest output for the default > > > output_format (html) as per conversation in mailing list. Then this script > > will > > > not need to override output_format at all. > > > > The wrapper script is confusing and won't work on Windows. We could just as > > easily override this option in run_gpu_test.py if it isn't specified on the > > command line. It would be preferable to land this CL without the wrapper > script. > > In run_gpu_test.py: > sys.exit(test_runner.main(sys.argv + ['--output-format', 'gtest'])) Yeah, this will work. Thanks for the suggestion! At this point, I'm thinking of waiting til I can get my patch on enabling progress reporting for all output format to a landable state. It's still blocking on another patch right now. If that works, we can drop this patch. If that hits a snag...
The other patch has landed: https://codereview.chromium.org/430383003/ Closing this patch since it is not required anymore. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
