Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2590623002:
[Telemetry] Migrate browser_test_runner to use typ as the test runner (Closed)
Description[Telemetry] Migrate browser_test_runner to use typ as the test runner
BUG=chromium:636153
Review-Url: https://codereview.chromium.org/2590623002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/34e664ad6a6e3fc23f06db94ab55f0e94b8027a6
Patch Set 1 #
Total comments: 3
Patch Set 2 : Keep all the sharding logic #Patch Set 3 : Reformat to make diff more similar #Patch Set 4 : Reformat to make diff more similar #Patch Set 5 : Add missing options for typ #
Total comments: 10
Patch Set 6 : update #Patch Set 7 : Update unittests #
Total comments: 4
Patch Set 8 : Expand test suffixes to include '_tests' and 'unittests' #Patch Set 9 : typo #Patch Set 10 : Add client_configs to the context #
Total comments: 1
Patch Set 11 : Making sure browser_test_runner does not pick up unittest #
Total comments: 9
Patch Set 12 : Rebase #Patch Set 13 : Address Ken's nits #Patch Set 14 : Rebase & fix TestLoadAllTestModules.testLoadAllTestsInModule #Messages
Total messages: 63 (28 generated)
nednguyen@google.com changed reviewers: + dpranke@chromium.org, kbr@chromium.org
./telemetry/bin/run_browser_tests SimpleShardingTest -v is now working with this CL However, I still need to port some of the functionality to typ, including: 1) "--filter-tests-after-sharding": Apply the test filter after tests are split for sharding. Useful for reproducing bugs related to the order in which tests run. 2) "--debug-shard-distributions": Print debugging information about the shards' test distributions 3) read_abbreviated_json_results_from: allow the harness to read the timing info of tests so it can distribute tests into shards better. Please take a high level review on this CL, 1) & 2) & 3) above probably would be done as separate CLs to typ.
Thanks for pushing this forward Ned. Supporting (3) when switching to typ is a hard requirement. Otherwise the shards for the WebGL 2.0 conformance tests will become very imbalanced. There are some long running tests in there and a lot of short running ones. If they're simply sorted by name they'll be highly imbalanced. Even random shards are not that well balanced. The current sharding scheme yields an almost perfect shard distribution. I don't know how typ discovers the tests to run. Isn't there a loading hook in the tests themselves? Can that be modified to report only a sub-portion of the available tests to typ? It would make things more maintainable if the logic stayed outside of typ. The specific order in which the tests run isn't *that* important, if that can't be controlled outside of typ. https://codereview.chromium.org/2590623002/diff/1/telemetry/telemetry/testing... File telemetry/telemetry/testing/serially_executed_browser_test_case.py (right): https://codereview.chromium.org/2590623002/diff/1/telemetry/telemetry/testing... telemetry/telemetry/testing/serially_executed_browser_test_case.py:150: continue What's this if-test for? Looks a bit hacky.
https://codereview.chromium.org/2590623002/diff/1/telemetry/telemetry/testing... File telemetry/telemetry/testing/serially_executed_browser_test_case.py (right): https://codereview.chromium.org/2590623002/diff/1/telemetry/telemetry/testing... telemetry/telemetry/testing/serially_executed_browser_test_case.py:150: continue On 2016/12/19 22:29:34, Ken Russell wrote: > What's this if-test for? Looks a bit hacky. This is to avoid calling test generation when the test class is not target test. This is required, otherwise different test classes can customize the test option parser differently (using test_class.AddCommandlineArgs(parser)) & fails at the test generation step because they look for options that were not defined in the parser.
https://codereview.chromium.org/2590623002/diff/1/telemetry/telemetry/testing... File telemetry/telemetry/testing/serially_executed_browser_test_case.py (right): https://codereview.chromium.org/2590623002/diff/1/telemetry/telemetry/testing... telemetry/telemetry/testing/serially_executed_browser_test_case.py:150: continue On 2016/12/19 22:54:17, nednguyen wrote: > On 2016/12/19 22:29:34, Ken Russell wrote: > > What's this if-test for? Looks a bit hacky. > > This is to avoid calling test generation when the test class is not target test. > This is required, otherwise different test classes can customize the test option > parser differently (using test_class.AddCommandlineArgs(parser)) & fails at the > test generation step because they look for options that were not defined in the > parser. Thanks for the info. This is definitely worth a comment. Could you please add one?
This basically lgtm . I didn't sweat the details of the review, as I figured kbr@ was doing so; let me know if you want me to as well. I don't understand everything in comment #2, but presumably we can follow up on that elsewhere? If you'd prefer to discuss in this CL, I can post my questions.
On 2016/12/20 03:08:41, Dirk Pranke wrote: > This basically lgtm . I didn't sweat the details of the review, as I figured > kbr@ was doing so; let me know if you want me to as well. > > I don't understand everything in comment #2, but presumably we can follow up on > that elsewhere? If you'd prefer to discuss in this CL, I can post my questions. We can certainly follow up offline, but this does not lgtm in its current form. The current harness's sharding support can not regress in the manner this CL suggests.
On 2016/12/20 05:52:21, Ken Russell wrote: > On 2016/12/20 03:08:41, Dirk Pranke wrote: > > This basically lgtm . I didn't sweat the details of the review, as I figured > > kbr@ was doing so; let me know if you want me to as well. > > > > I don't understand everything in comment #2, but presumably we can follow up > on > > that elsewhere? If you'd prefer to discuss in this CL, I can post my > questions. > > We can certainly follow up offline, but this does not lgtm in its current form. > The current harness's sharding support can not regress in the manner this CL > suggests. I think the way to move forward here without blocking on typ support for sharding based on test times is that I will reuse the list of ids of tests to be run from the main process & pass it to typ subprocesses through context object & then have subprocesses run only tests whose ids matched with those.
PTAL again, this patch keeps all the sharding logic in run_browser_test.py instead of delegate to typ. This allows us to keep *almost* all the functionality the same. *almost* because I still need to support transform the full result to abbreviated json so that this patch is landable & not disrupt the GPU tests. Ken: if you're happy with the general structure here, can you work on adding the boiler-plate load_tests(..) to the gpu tests that are similar to https://codereview.chromium.org/2562913003?
Thanks, Ned, this looks much better than before. Thumbs up on the overall direction. I can't approve this CL until I see that the per-test times are wired up. That's crucial to keeping our current tests running. If we need to temporarily massage JSON files into and out of typ, I will be happy to own removing that, if you can add it in. https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... File telemetry/telemetry/testing/browser_test_context.py (right): https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/browser_test_context.py:22: _ test_cases_ids_to_run: the ids of the test cases to be run. Grammar: here and throughout: test_cases_ids -> test_case_ids Could you document what these are? i.e., provide some example? https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... File telemetry/telemetry/testing/run_browser_tests.py (right): https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/run_browser_tests.py:281: # by passing browser_test_context.test_cases_to_run_ids to subprocess to through by -> by https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/run_browser_tests.py:282: # indicate test cases to be run, we explicitly disable sharding handling sharding handling -> sharding https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... File telemetry/telemetry/testing/serially_executed_browser_test_case.py (right): https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/serially_executed_browser_test_case.py:149: # We bail out early if the test class's name doesn't match with targeted match with -> match the https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/serially_executed_browser_test_case.py:153: # find their customed options in finder_options object. customed -> custom Also, if the basic idea here is to avoid calling GenerateTestCases for tests that we don't actually intend to run, could you mention that explicitly?
Also, Ned, it's difficult for me/us to add the boilerplate logic you describe in https://codereview.chromium.org/2562913003 . We can't test it without this patched into Telemetry, and this CL isn't done yet. The addition of this boilerplate should be small and simple, and must be done along with the Catapult roll which pulls in these changes. Could you please own that task? I don't think it's easily separable.
On 2016/12/21 00:12:59, Ken Russell wrote: > Thanks, Ned, this looks much better than before. Thumbs up on the overall > direction. > > I can't approve this CL until I see that the per-test times are wired up. That's > crucial to keeping our current tests running. The perf-test time are wired up in run_browser_tests.LoadTestCasesToBeRun > If we need to temporarily massage > JSON files into and out of typ, I will be happy to own removing that, if you can > add it in. > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > File telemetry/telemetry/testing/browser_test_context.py (right): > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > telemetry/telemetry/testing/browser_test_context.py:22: _ test_cases_ids_to_run: > the ids of the test cases to be run. > Grammar: here and throughout: test_cases_ids -> test_case_ids > > Could you document what these are? i.e., provide some example? > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > File telemetry/telemetry/testing/run_browser_tests.py (right): > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > telemetry/telemetry/testing/run_browser_tests.py:281: # by passing > browser_test_context.test_cases_to_run_ids to subprocess to > through by -> by > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > telemetry/telemetry/testing/run_browser_tests.py:282: # indicate test cases to > be run, we explicitly disable sharding handling > sharding handling -> sharding > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > File telemetry/telemetry/testing/serially_executed_browser_test_case.py (right): > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > telemetry/telemetry/testing/serially_executed_browser_test_case.py:149: # We > bail out early if the test class's name doesn't match with targeted > match with -> match the > > https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... > telemetry/telemetry/testing/serially_executed_browser_test_case.py:153: # find > their customed options in finder_options object. > customed -> custom > > Also, if the basic idea here is to avoid calling GenerateTestCases for tests > that we don't actually intend to run, could you mention that explicitly?
On 2016/12/21 00:14:19, Ken Russell wrote: > Also, Ned, it's difficult for me/us to add the boilerplate logic you describe in > https://codereview.chromium.org/2562913003 . We can't test it without this > patched into Telemetry, and this CL isn't done yet. FWIW CL isn't done in the sense that I haven't fixed the corresponding unittest. But functionality wise, it should be ready for you to run the integration gpu test. e.g: you can patch this CL & run: ./telemetry/bin/run_browser_tests SimpleBrowserTest -v --write-full-results-to=foo.json > The addition of this > boilerplate should be small and simple, and must be done along with the Catapult > roll which pulls in these changes. Could you please own that task? I don't think > it's easily separable. I can do that. But I think it's still best that you patch this in chromium/src/third_party/catapult & try to run gpu integration tests with this. You are much better at detecting anomalies with running the gpu integration tests than me.
https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... File telemetry/telemetry/testing/browser_test_context.py (right): https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/browser_test_context.py:22: _ test_cases_ids_to_run: the ids of the test cases to be run. On 2016/12/21 00:12:59, Ken Russell wrote: > Grammar: here and throughout: test_cases_ids -> test_case_ids > > Could you document what these are? i.e., provide some example? Done. https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... File telemetry/telemetry/testing/run_browser_tests.py (right): https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/run_browser_tests.py:281: # by passing browser_test_context.test_cases_to_run_ids to subprocess to On 2016/12/21 00:12:59, Ken Russell wrote: > through by -> by Done. https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/run_browser_tests.py:282: # indicate test cases to be run, we explicitly disable sharding handling On 2016/12/21 00:12:59, Ken Russell wrote: > sharding handling -> sharding Done. https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... File telemetry/telemetry/testing/serially_executed_browser_test_case.py (right): https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/serially_executed_browser_test_case.py:149: # We bail out early if the test class's name doesn't match with targeted On 2016/12/21 00:12:59, Ken Russell wrote: > match with -> match the Done. https://codereview.chromium.org/2590623002/diff/80001/telemetry/telemetry/tes... telemetry/telemetry/testing/serially_executed_browser_test_case.py:153: # find their customed options in finder_options object. On 2016/12/21 00:12:59, Ken Russell wrote: > customed -> custom > > Also, if the basic idea here is to avoid calling GenerateTestCases for tests > that we don't actually intend to run, could you mention that explicitly? Done.
All the unittests are passing now, except the two tests about error in setUp & tearDown https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... File telemetry/telemetry/testing/browser_test_runner_unittest.py (right): https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... telemetry/telemetry/testing/browser_test_runner_unittest.py:86: def testJsonOutputWhenSetupClassFailed(self): Dirk: this test is failing because typ cannot output full json result in this case https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... telemetry/telemetry/testing/browser_test_runner_unittest.py:92: def testJsonOutputWhenTearDownClassFailed(self): Same here
https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... File telemetry/telemetry/testing/browser_test_runner_unittest.py (right): https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... telemetry/telemetry/testing/browser_test_runner_unittest.py:86: def testJsonOutputWhenSetupClassFailed(self): On 2016/12/22 02:29:03, nednguyen wrote: > Dirk: this test is failing because typ cannot output full json result in this > case What error are you getting?
https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... File telemetry/telemetry/testing/browser_test_runner_unittest.py (right): https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... telemetry/telemetry/testing/browser_test_runner_unittest.py:86: def testJsonOutputWhenSetupClassFailed(self): On 2016/12/22 21:21:17, Dirk Pranke wrote: > On 2016/12/22 02:29:03, nednguyen wrote: > > Dirk: this test is failing because typ cannot output full json result in this > > case > > What error are you getting? Oh, after digging further to see the error, it turns out that typ by default only accept files with "_test.py" & "_unittest.py" whereas my test file is "_tests.py". After fixing it, things are ok now.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/23 01:17:57, nednguyen wrote: > https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... > File telemetry/telemetry/testing/browser_test_runner_unittest.py (right): > > https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... > telemetry/telemetry/testing/browser_test_runner_unittest.py:86: def > testJsonOutputWhenSetupClassFailed(self): > On 2016/12/22 21:21:17, Dirk Pranke wrote: > > On 2016/12/22 02:29:03, nednguyen wrote: > > > Dirk: this test is failing because typ cannot output full json result in > this > > > case > > > > What error are you getting? > > Oh, after digging further to see the error, it turns out that typ by default > only accept files with "_test.py" & "_unittest.py" whereas my test file is > "_tests.py". After fixing it, things are ok now. Ken: ping
On 2017/01/04 13:33:36, nednguyen wrote: > On 2016/12/23 01:17:57, nednguyen wrote: > > > https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... > > File telemetry/telemetry/testing/browser_test_runner_unittest.py (right): > > > > > https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... > > telemetry/telemetry/testing/browser_test_runner_unittest.py:86: def > > testJsonOutputWhenSetupClassFailed(self): > > On 2016/12/22 21:21:17, Dirk Pranke wrote: > > > On 2016/12/22 02:29:03, nednguyen wrote: > > > > Dirk: this test is failing because typ cannot output full json result in > > this > > > > case > > > > > > What error are you getting? > > > > Oh, after digging further to see the error, it turns out that typ by default > > only accept files with "_test.py" & "_unittest.py" whereas my test file is > > "_tests.py". After fixing it, things are ok now. > > Ken: ping Ping Ken again. We are very close to get this work done. All that left is to add the required boiler plate code to gpu tests.
On 2017/01/06 14:31:54, nednguyen wrote: > On 2017/01/04 13:33:36, nednguyen wrote: > > On 2016/12/23 01:17:57, nednguyen wrote: > > > > > > https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... > > > File telemetry/telemetry/testing/browser_test_runner_unittest.py (right): > > > > > > > > > https://codereview.chromium.org/2590623002/diff/120001/telemetry/telemetry/te... > > > telemetry/telemetry/testing/browser_test_runner_unittest.py:86: def > > > testJsonOutputWhenSetupClassFailed(self): > > > On 2016/12/22 21:21:17, Dirk Pranke wrote: > > > > On 2016/12/22 02:29:03, nednguyen wrote: > > > > > Dirk: this test is failing because typ cannot output full json result in > > > this > > > > > case > > > > > > > > What error are you getting? > > > > > > Oh, after digging further to see the error, it turns out that typ by default > > > only accept files with "_test.py" & "_unittest.py" whereas my test file is > > > "_tests.py". After fixing it, things are ok now. > > > > Ken: ping > > Ping Ken again. We are very close to get this work done. All that left is to add > the required boiler plate code to gpu tests. I'm sorry for not looking at this again yet. I'm actively trying to get all of the GPU tests onto the browser_test_runner so that we can switch them all to typ at once. See http://crbug.com/352807 . There's only one remaining (Maps) and I intend to port it today.
As I noted in https://codereview.chromium.org/2616253002/#msg4 , I think I need to actually review this, since it looks like the integration w/ typ wasn't done the way I'd expect it to be done. Should this patch actually work as-is? If I cd to third_party/catapult, apply this patch to tip-of-tree, and try to run `python telemetry/bin/run_browser_tests`, it fails.
On 2017/01/09 03:17:15, Dirk Pranke wrote: > As I noted in https://codereview.chromium.org/2616253002/#msg4 , I think I need > to actually review this, > since it looks like the integration w/ typ wasn't done the way I'd expect it to > be done. > > Should this patch actually work as-is? > > If I cd to third_party/catapult, apply this patch to tip-of-tree, and try to run > `python telemetry/bin/run_browser_tests`, > it fails. Hi Dirk, you need to specify which test to run (we probably should have some line that says "pick a test" to run in the prompt). An example command to run is "./telemetry/bin/run_browser_tests SimpleBrowserTest --browser=system"
Just for the record, https://codereview.chromium.org/2616253002/ has the typ boilerplate added to the GPU correctness tests, so if you apply that locally you could also run e.g.: ./content/test/gpu/run_gpu_integration_test.py context_lost --browser=canary and see that it incorrectly runs the unit tests in that directory as opposed to just the context_lost tests.
https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... File telemetry/telemetry/testing/run_browser_tests.py (right): https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... telemetry/telemetry/testing/run_browser_tests.py:24: TEST_SUFFIXES = ['*_test.py', '*_tests.py', '*_unittest.py', '*_unittests.py'] Ken, Dirk: I can exclude _unittest to fix the bug you mention. Do you think it's fragile though?
On 2017/01/09 22:25:00, nednguyen wrote: > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > File telemetry/telemetry/testing/run_browser_tests.py (right): > > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > telemetry/telemetry/testing/run_browser_tests.py:24: TEST_SUFFIXES = > ['*_test.py', '*_tests.py', '*_unittest.py', '*_unittests.py'] > Ken, Dirk: I can exclude _unittest to fix the bug you mention. Do you think it's > fragile though? Yes, that sounds fragile. Please work with Dirk to see whether it's possible to more directly select the tests to run. It looks like there is a two-step discovery process. The one here knows that it's looking for subclasses of SeriallyExecutedBrowserTestCase, but typ doesn't know that. That seems like an omission.
On 2017/01/09 22:31:19, Ken Russell wrote: > On 2017/01/09 22:25:00, nednguyen wrote: > > > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > > File telemetry/telemetry/testing/run_browser_tests.py (right): > > > > > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > > telemetry/telemetry/testing/run_browser_tests.py:24: TEST_SUFFIXES = > > ['*_test.py', '*_tests.py', '*_unittest.py', '*_unittests.py'] > > Ken, Dirk: I can exclude _unittest to fix the bug you mention. Do you think > it's > > fragile though? > > Yes, that sounds fragile. Please work with Dirk to see whether it's possible to > more directly select the tests to run. It looks like there is a two-step > discovery process. The one here knows that it's looking for subclasses of > SeriallyExecutedBrowserTestCase, but typ doesn't know that. That seems like an > omission. I think the way to fix this is probably to audit the filtering logic to be more aggressive about selecting only test that's recognized by the run_browser_test's main process.
On 2017/01/11 21:03:55, nednguyen wrote: > On 2017/01/09 22:31:19, Ken Russell wrote: > > On 2017/01/09 22:25:00, nednguyen wrote: > > > > > > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > > > File telemetry/telemetry/testing/run_browser_tests.py (right): > > > > > > > > > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > > > telemetry/telemetry/testing/run_browser_tests.py:24: TEST_SUFFIXES = > > > ['*_test.py', '*_tests.py', '*_unittest.py', '*_unittests.py'] > > > Ken, Dirk: I can exclude _unittest to fix the bug you mention. Do you think > > it's > > > fragile though? > > > > Yes, that sounds fragile. Please work with Dirk to see whether it's possible > to > > more directly select the tests to run. It looks like there is a two-step > > discovery process. The one here knows that it's looking for subclasses of > > SeriallyExecutedBrowserTestCase, but typ doesn't know that. That seems like an > > omission. > > I think the way to fix this is probably to audit the filtering logic to be more > aggressive about selecting only test that's recognized by the run_browser_test's > main process. Sounds fine; if you could work directly with Dirk on that I'm happy to re-test the GPU tests with any changes.
On 2017/01/12 00:10:02, Ken Russell wrote: > On 2017/01/11 21:03:55, nednguyen wrote: > > On 2017/01/09 22:31:19, Ken Russell wrote: > > > On 2017/01/09 22:25:00, nednguyen wrote: > > > > > > > > > > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > > > > File telemetry/telemetry/testing/run_browser_tests.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2590623002/diff/180001/telemetry/telemetry/te... > > > > telemetry/telemetry/testing/run_browser_tests.py:24: TEST_SUFFIXES = > > > > ['*_test.py', '*_tests.py', '*_unittest.py', '*_unittests.py'] > > > > Ken, Dirk: I can exclude _unittest to fix the bug you mention. Do you > think > > > it's > > > > fragile though? > > > > > > Yes, that sounds fragile. Please work with Dirk to see whether it's possible > > to > > > more directly select the tests to run. It looks like there is a two-step > > > discovery process. The one here knows that it's looking for subclasses of > > > SeriallyExecutedBrowserTestCase, but typ doesn't know that. That seems like > an > > > omission. > > > > I think the way to fix this is probably to audit the filtering logic to be > more > > aggressive about selecting only test that's recognized by the > run_browser_test's > > main process. > > Sounds fine; if you could work directly with Dirk on that I'm happy to re-test > the GPU tests with any changes. PTAL again, the code has been changed to make sure that it never pick up normal unittest (see the diff between patch 11 & 10). I also add sample_unittest.py to telemetry/examples/browser_tests/. That ensure the browser_test_runner_unittest will fail if it also discovers normal unittest (verified that undo the change in patch 11 does indeed makes the test fail).
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for pushing this forward Ned. It LGTM overall now, but I haven't tried running this with the GPU tests. Apologies, but I probably won't be able to do this for about a week (traveling on business until next Thursday). I don't know the typ APIs nor whether this is the recommended way to filter the tests which are executed. I defer to Dirk's review for that. A couple of minor typos. https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... File telemetry/telemetry/testing/browser_test_runner_unittest.py (right): https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/browser_test_runner_unittest.py:23: def _ExtracTestResults(self, test_result): Extrac -> Extract https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/browser_test_runner_unittest.py:335: class ErrorneousGeometric( Errorneous -> Erroneous https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/browser_test_runner_unittest.py:363: # This should not invoke GenerateTestCases of ErrorneousGeometric class, Same typo here. https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... File telemetry/telemetry/testing/run_browser_tests.py (right): https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/run_browser_tests.py:223: # Do not pick up tests that are not inherit from are not -> do not https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/run_browser_tests.py:234: # For now, only support running these tests in serial. in serial -> serially https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/run_browser_tests.py:337: binary_manager.InitDependencyManager(context.client_configs) Is this known behavior and/or something that we should investigate further in the future?
Note: I'd prefer to wait until a couple of days after http://crbug.com/682819 is fixed and the GPU tests are reliably green again before switching to typ. We have to start from a known good state with regard to test flakiness.
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... File telemetry/telemetry/testing/run_browser_tests.py (right): https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/run_browser_tests.py:223: # Do not pick up tests that are not inherit from On 2017/01/20 02:50:28, Ken Russell wrote: > are not -> do not Done. https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/run_browser_tests.py:234: # For now, only support running these tests in serial. On 2017/01/20 02:50:28, Ken Russell wrote: > in serial -> serially Done. https://codereview.chromium.org/2590623002/diff/200001/telemetry/telemetry/te... telemetry/telemetry/testing/run_browser_tests.py:337: binary_manager.InitDependencyManager(context.client_configs) On 2017/01/20 02:50:28, Ken Russell wrote: > Is this known behavior and/or something that we should investigate further in > the future? Yes. It's a known workaround because window's doesn't have fork like unix based, so we need to launch a new process from scratch & message pass the data to it.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2590623002/#ps240001 (title: "Address Ken's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2590623002/#ps260001 (title: "Rebase & fix TestLoadAllTestModules.testLoadAllTestsInModule")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1485868661781690,
"parent_rev": "6bc0354c35405b02db027aa20976c14983d21955", "commit_rev":
"34e664ad6a6e3fc23f06db94ab55f0e94b8027a6"}
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Migrate browser_test_runner to use typ as the test runner BUG=chromium:636153 ========== to ========== [Telemetry] Migrate browser_test_runner to use typ as the test runner BUG=chromium:636153 Review-Url: https://codereview.chromium.org/2590623002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2662193004/ by nednguyen@google.com. The reason for reverting is: This is not ready to land yet. Revert to unblock rolling out other catapult commits.. |
