Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1354223004:
Run telemetry_gpu_unittests via isolate on "Linux Tests" and trybot. (Closed)
|
Created:
5 years, 3 months ago by nednguyen Modified:
5 years, 2 months ago Reviewers:
Dirk Pranke, Paweł Hajdan Jr., M-A Ruel, Ken Russell (switch to Gerrit), nednguyen(REVIEW IN OTHER ACC) CC:
Paweł Hajdan Jr., chromium-reviews, eakuefner, iannucci Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRun telemetry_gpu_unittests via isolate on "Linux Tests" and trybot.
This changes the "Linux Tests" bot and associated trybot to build the
new telemetry_gpu_unittests isolate, and run that step via the
isolate.
This is just a first step to make sure the code is being exercised.
It's been tested locally but only on this configuration. Some more
work might be needed to get this working in non-GN builds. Further
refactoring of the Telemetry dependencies will occur in follow-on CLs.
BUG=507796
Committed: https://crrev.com/fbee71a6424ca43fbf8b87add04f132a83abe64c
Cr-Commit-Position: refs/heads/master@{#351784}
Patch Set 1 #Patch Set 2 : Fix PRESUBMIT #Patch Set 3 : Undo change to mb_config.pyl #Patch Set 4 : Fix mb.py's fix #
Total comments: 8
Patch Set 5 : Address review comments #Patch Set 6 : Temporarily add debugging #Patch Set 7 : Add more debugging message in runtest_wraper.py #Patch Set 8 : Add fix to runtest_wrapper.py #Patch Set 9 : Add chrome to telemetry_gpu_unittests' dep #Patch Set 10 : Clean up debug printing #
Total comments: 7
Patch Set 11 : Rebase #Patch Set 12 : Add third_party/webgl/src/sdk/tests #
Messages
Total messages: 42 (5 generated)
nednguyen@google.com changed reviewers: + dpranke@chromium.org, kbr@chromium.org
kbr@chromium.org changed reviewers: + maruel@chromium.org
I updated the description to be more clear what's going on here. We're eagerly looking forward to landing this so we can follow it up with more CLs refactoring Telemetry's dependencies in the GN build for other bugs (crbug.com/466877 specifically), as well as testing it in more configurations. This one's known to work so we'd like to start with it, rather than trying to make all of the bots work all at once. https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py File testing/buildbot/manage.py (right): https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', I don't think this is needed. Could you please take it out and re-send the CQ dry run? https://codereview.chromium.org/1354223004/diff/60001/testing/scripts/run_tel... File testing/scripts/run_telemetry_as_googletest.py (right): https://codereview.chromium.org/1354223004/diff/60001/testing/scripts/run_tel... testing/scripts/run_telemetry_as_googletest.py:38: print 'Writing results to %s' % args.isolated_script_test_output Please remove debugging print statement.
basically looks fine, but your test step failed w/ invalid results on the linux_chromium_rel_ng tryjob, so you'll want to figure out what's wrong w/ that. https://codereview.chromium.org/1354223004/diff/60001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1354223004/diff/60001/tools/mb/mb.py#newcode559 tools/mb/mb.py:559: ] + gn_isolate_map[target].get('args', []) nit: can you reformat this to: cmdline = [ '../../testing/test_env.py', '../../' + self.ToSrcRelPath(gn_isolate_map[target]['script']), ] + gn_isolate_map[target].get('args', []) ?
On 2015/09/19 00:07:13, Dirk Pranke wrote: > basically looks fine, but your test step failed w/ invalid results on the > linux_chromium_rel_ng tryjob, so you'll want to figure out what's wrong w/ that. Yes. We ran the recipe locally on Ned's Linux workstation with the recipe-side change; not sure what's different and caused this to break. http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... The failure is: ----- /usr/bin/python /b/build/slave/linux/build/src/tools/swarming_client/run_isolated.py --isolated e8b4613401cda41a6905579b2c7dd8e63d901873 -I https://isolateserver.appspot.com --isolated-script-test-output /tmp/tmpy_NGO1.json Usage: run_isolated.py <options> run_isolated.py: error: no such option: --isolated-script-test-output ----- There's a missing "--" before --isolated-script-test-output, which was present when runtest_wrapper.py was invoked: ----- python -u /b/build/slave/linux/build/src/infra/scripts/runtest_wrapper.py ... --run-python-script /b/build/slave/linux/build/src/tools/swarming_client/run_isolated.py --isolated e8b4613401cda41a6905579b2c7dd8e63d901873 -I https://isolateserver.appspot.com -- --isolated-script-test-output /tmp/tmpy_NGO1.json -----
On 2015/09/19 00:51:44, Ken Russell wrote: > On 2015/09/19 00:07:13, Dirk Pranke wrote: > > basically looks fine, but your test step failed w/ invalid results on the > > linux_chromium_rel_ng tryjob, so you'll want to figure out what's wrong w/ > that. > > Yes. We ran the recipe locally on Ned's Linux workstation with the recipe-side > change; not sure what's different and caused this to break. > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > The failure is: > > ----- > /usr/bin/python > /b/build/slave/linux/build/src/tools/swarming_client/run_isolated.py --isolated > e8b4613401cda41a6905579b2c7dd8e63d901873 -I https://isolateserver.appspot.com > --isolated-script-test-output /tmp/tmpy_NGO1.json > Usage: run_isolated.py <options> > > run_isolated.py: error: no such option: --isolated-script-test-output > ----- > > There's a missing "--" before --isolated-script-test-output, which was present > when runtest_wrapper.py was invoked: > > ----- > python -u /b/build/slave/linux/build/src/infra/scripts/runtest_wrapper.py ... > --run-python-script > /b/build/slave/linux/build/src/tools/swarming_client/run_isolated.py --isolated > e8b4613401cda41a6905579b2c7dd8e63d901873 -I https://isolateserver.appspot.com -- > --isolated-script-test-output /tmp/tmpy_NGO1.json > ----- Ned and I looked through the logs from the run of the recipe on his local machine. runtest_wrapper.py is invoked in the same way, but when it invokes run_isolated.py, the "--" before --isolated-script-test-output is present. Any ideas why there's a difference between a local run of the recipe and that on the tryserver? +iannucci
I see multiple '--' in that command line... is it possible that one or both wrappers are parsing their arguments incorrectly? On Fri, Sep 18, 2015 at 6:24 PM <kbr@chromium.org> wrote: > On 2015/09/19 00:51:44, Ken Russell wrote: > > On 2015/09/19 00:07:13, Dirk Pranke wrote: > > > basically looks fine, but your test step failed w/ invalid results on > > the > > > linux_chromium_rel_ng tryjob, so you'll want to figure out what's wrong > > w/ > > that. > > > Yes. We ran the recipe locally on Ned's Linux workstation with the > > recipe-side > > change; not sure what's different and caused this to break. > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > > The failure is: > > > ----- > > /usr/bin/python > > /b/build/slave/linux/build/src/tools/swarming_client/run_isolated.py > --isolated > > e8b4613401cda41a6905579b2c7dd8e63d901873 -I > > https://isolateserver.appspot.com > > --isolated-script-test-output /tmp/tmpy_NGO1.json > > Usage: run_isolated.py <options> > > > run_isolated.py: error: no such option: --isolated-script-test-output > > ----- > > > There's a missing "--" before --isolated-script-test-output, which was > > present > > when runtest_wrapper.py was invoked: > > > ----- > > python -u > > /b/build/slave/linux/build/src/infra/scripts/runtest_wrapper.py ... > > --run-python-script > > /b/build/slave/linux/build/src/tools/swarming_client/run_isolated.py > --isolated > > e8b4613401cda41a6905579b2c7dd8e63d901873 -I > > https://isolateserver.appspot.com > -- > > --isolated-script-test-output /tmp/tmpy_NGO1.json > > ----- > > Ned and I looked through the logs from the run of the recipe on his local > machine. runtest_wrapper.py is invoked in the same way, but when it invokes > run_isolated.py, the "--" before --isolated-script-test-output is present. > > Any ideas why there's a difference between a local run of the recipe and > that on > the tryserver? +iannucci > > > https://codereview.chromium.org/1354223004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/09/19 02:05:03, iannucci wrote: > I see multiple '--' in that command line... is it possible that one or both > wrappers are parsing their arguments incorrectly? Certainly possible. This is deliberate; it looks like runtest_wrapper.py is using '--' itself, and so is this new step. The question is still why the recipe run would have succeeded on Ned's machine and failed on the bots. I thought run_recipe was supposed to emulate nearly all of the behaviors on the bots. The output from the step invocation on Ned's machine has been added to https://code.google.com/p/chromium/issues/detail?id=507796#c10 . It shows that the second '--' is passed through correctly.
Hm... if you actually ran it with 'run_recipe.py' and it worked on your machine, then that's confusing to me. I do know that chromium*.py have use some (very) liberal fake data for their simulations. Pawel, do you know what could be going wrong? On Sun, Sep 20, 2015 at 9:44 AM <kbr@chromium.org> wrote: > On 2015/09/19 02:05:03, iannucci wrote: > > I see multiple '--' in that command line... is it possible that one or > > both > > wrappers are parsing their arguments incorrectly? > > Certainly possible. This is deliberate; it looks like runtest_wrapper.py is > using '--' itself, and so is this new step. The question is still why the > recipe > run would have succeeded on Ned's machine and failed on the bots. I thought > run_recipe was supposed to emulate nearly all of the behaviors on the bots. > The > output from the step invocation on Ned's machine has been added to > https://code.google.com/p/chromium/issues/detail?id=507796#c10 . It shows > that > the second '--' is passed through correctly. > > > https://codereview.chromium.org/1354223004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/09/20 18:20:12, iannucci wrote: > Hm... if you actually ran it with 'run_recipe.py' and it worked on your > machine, then that's confusing to me. I do know that chromium*.py have use > some (very) liberal fake data for their simulations. Pawel, do you know > what could be going wrong? To confirm, yes, we ran this with run_recipe.py. We made a fake test machine configuration so that we could run just this one new step, and ran the entire recipe, including checking out the fake_slave's workspace and building it (I didn't know how to skip the build step, even though we didn't really need it). That's the output that's attached to https://code.google.com/p/chromium/issues/detail?id=507796#c10 . Is there a difference between the runtest.py that would be used when running the recipe locally, and that used when on the bots? Is one in the infra workspace and one in the Chromium workspace?
No there's no difference unless the input parameters (properties) are different. run_recipe is just a thin wrapper around annotated_run, which is what the bot runs. Environment and everything else should match too. On Sun, Sep 20, 2015, 11:35 <kbr@chromium.org> wrote: > On 2015/09/20 18:20:12, iannucci wrote: > > Hm... if you actually ran it with 'run_recipe.py' and it worked on your > > machine, then that's confusing to me. I do know that chromium*.py have > use > > some (very) liberal fake data for their simulations. Pawel, do you know > > what could be going wrong? > > To confirm, yes, we ran this with run_recipe.py. We made a fake test > machine > configuration so that we could run just this one new step, and ran the > entire > recipe, including checking out the fake_slave's workspace and building it > (I > didn't know how to skip the build step, even though we didn't really need > it). > That's the output that's attached to > https://code.google.com/p/chromium/issues/detail?id=507796#c10 . > > Is there a difference between the runtest.py that would be used when > running the > recipe locally, and that used when on the bots? Is one in the infra > workspace > and one in the Chromium workspace? > > > https://codereview.chromium.org/1354223004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py File testing/buildbot/manage.py (right): https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', On 2015/09/18 23:52:51, Ken Russell wrote: > I don't think this is needed. Could you please take it out and re-send the CQ > dry run? Done. https://codereview.chromium.org/1354223004/diff/60001/testing/scripts/run_tel... File testing/scripts/run_telemetry_as_googletest.py (right): https://codereview.chromium.org/1354223004/diff/60001/testing/scripts/run_tel... testing/scripts/run_telemetry_as_googletest.py:38: print 'Writing results to %s' % args.isolated_script_test_output On 2015/09/18 23:52:51, Ken Russell wrote: > Please remove debugging print statement. Done. https://codereview.chromium.org/1354223004/diff/60001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1354223004/diff/60001/tools/mb/mb.py#newcode559 tools/mb/mb.py:559: ] + gn_isolate_map[target].get('args', []) On 2015/09/19 00:07:13, Dirk Pranke wrote: > nit: can you reformat this to: > > cmdline = [ > '../../testing/test_env.py', > '../../' + self.ToSrcRelPath(gn_isolate_map[target]['script']), > ] + gn_isolate_map[target].get('args', []) > > ? Done.
https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py File testing/buildbot/manage.py (right): https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', On 2015/09/21 17:26:18, nednguyen wrote: > On 2015/09/18 23:52:51, Ken Russell wrote: > > I don't think this is needed. Could you please take it out and re-send the CQ > > dry run? > > Done. Actually, this is needed to bypass the presubmit check. Without this, I got the complaint: ** Presubmit ERRORS ** manage (0.08s) failed telemetry_gpu_unittests is listed in gn_isolate_map.pyl but not in any .json files
https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py File testing/buildbot/manage.py (right): https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', On 2015/09/21 17:29:12, nednguyen wrote: > On 2015/09/21 17:26:18, nednguyen wrote: > > On 2015/09/18 23:52:51, Ken Russell wrote: > > > I don't think this is needed. Could you please take it out and re-send the > CQ > > > dry run? > > > > Done. > > Actually, this is needed to bypass the presubmit check. Without this, I got the > complaint: > > ** Presubmit ERRORS ** > manage (0.08s) failed > telemetry_gpu_unittests is listed in gn_isolate_map.pyl but not in any .json > files > Pawel, Dirk, do either of you know why that's happening? Does this new test need to be added elsewhere to the .json files? I don't see any place where build targets are listed separately from targets that would be run.
On 2015/09/21 20:59:39, Ken Russell wrote: > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py > File testing/buildbot/manage.py (right): > > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... > testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', > On 2015/09/21 17:29:12, nednguyen wrote: > > On 2015/09/21 17:26:18, nednguyen wrote: > > > On 2015/09/18 23:52:51, Ken Russell wrote: > > > > I don't think this is needed. Could you please take it out and re-send the > > CQ > > > > dry run? > > > > > > Done. > > > > Actually, this is needed to bypass the presubmit check. Without this, I got > the > > complaint: > > > > ** Presubmit ERRORS ** > > manage (0.08s) failed > > telemetry_gpu_unittests is listed in gn_isolate_map.pyl but not in any .json > > files > > > > Pawel, Dirk, do either of you know why that's happening? Does this new test need > to be added elsewhere to the .json files? I don't see any place where build > targets are listed separately from targets that would be run. Ken, after the fix in infra/scripts/runtest_wrapper.py, the log is showing a different failure: Running ['/usr/bin/python', '../../content/test/gpu/run_unittests.py', '-v', '--write-full-results-to', '/tmp/isolated_tmpbO4FrN/tmpVZBkZa'] libdc1394 error: Failed to initialize libdc1394 Usage: run_tests.py [test_name ...] [<options>] run_tests.py: error: No browser found of type any. Cannot run tests. Re-run with --browser=list to see available browser types. Command ['/usr/bin/python', '../../content/test/gpu/run_unittests.py', '-v', '--write-full-results-to', '/tmp/isolated_tmpbO4FrN/tmpVZBkZa'] returned exit code 2 Traceback (most recent call last): File "../../testing/scripts/run_telemetry_as_googletest.py", line 72, in <module> sys.exit(main()) File "../../testing/scripts/run_telemetry_as_googletest.py", line 44, in main results = json.load(f) File "/usr/lib/python2.7/json/__init__.py", line 278, in load **kw) File "/usr/lib/python2.7/json/__init__.py", line 326, in loads return _default_decoder.decode(s) File "/usr/lib/python2.7/json/decoder.py", line 366, in decode obj, end = self.raw_decode(s, idx=_w(s, 0).end()) File "/usr/lib/python2.7/json/decoder.py", line 384, in raw_decode raise ValueError("No JSON object could be decoded") ValueError: No JSON object could be decoded I think this could be because telemetry's run_tests.py requires the presence of the browser, and we don't hook up declare browser as dependency for telemetry_gpu_unittests in BUILD.gn. Filed crbug.com/534585
On 2015/09/21 20:59:39, Ken Russell wrote: > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py > File testing/buildbot/manage.py (right): > > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... > testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', > On 2015/09/21 17:29:12, nednguyen wrote: > > On 2015/09/21 17:26:18, nednguyen wrote: > > > On 2015/09/18 23:52:51, Ken Russell wrote: > > > > I don't think this is needed. Could you please take it out and re-send the > > CQ > > > > dry run? > > > > > > Done. > > > > Actually, this is needed to bypass the presubmit check. Without this, I got > the > > complaint: > > > > ** Presubmit ERRORS ** > > manage (0.08s) failed > > telemetry_gpu_unittests is listed in gn_isolate_map.pyl but not in any .json > > files > > > > Pawel, Dirk, do either of you know why that's happening? Does this new test need > to be added elsewhere to the .json files? I don't see any place where build > targets are listed separately from targets that would be run. Yup, there's a presubmit check that attempts to make sure the isolate map matches what we run. However, there are a few targets that don't map directly across to entries in the *.json files (because they are handled specially in the recipe), so there's a whitelist of exceptions in manage.py. In other words, Ned's change was in fact needed and correct.
On 2015/09/21 21:25:38, nednguyen wrote: > On 2015/09/21 20:59:39, Ken Russell wrote: > > > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py > > File testing/buildbot/manage.py (right): > > > > > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... > > testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', > > On 2015/09/21 17:29:12, nednguyen wrote: > > > On 2015/09/21 17:26:18, nednguyen wrote: > > > > On 2015/09/18 23:52:51, Ken Russell wrote: > > > > > I don't think this is needed. Could you please take it out and re-send > the > > > CQ > > > > > dry run? > > > > > > > > Done. > > > > > > Actually, this is needed to bypass the presubmit check. Without this, I got > > the > > > complaint: > > > > > > ** Presubmit ERRORS ** > > > manage (0.08s) failed > > > telemetry_gpu_unittests is listed in gn_isolate_map.pyl but not in any .json > > > files > > > > > > > Pawel, Dirk, do either of you know why that's happening? Does this new test > need > > to be added elsewhere to the .json files? I don't see any place where build > > targets are listed separately from targets that would be run. > > Ken, after the fix in infra/scripts/runtest_wrapper.py, the log is showing a > different failure: > > Running ['/usr/bin/python', '../../content/test/gpu/run_unittests.py', '-v', > '--write-full-results-to', '/tmp/isolated_tmpbO4FrN/tmpVZBkZa'] > libdc1394 error: Failed to initialize libdc1394 > Usage: run_tests.py [test_name ...] [<options>] > > run_tests.py: error: No browser found of type any. Cannot run tests. > Re-run with --browser=list to see available browser types. > Command ['/usr/bin/python', '../../content/test/gpu/run_unittests.py', '-v', > '--write-full-results-to', '/tmp/isolated_tmpbO4FrN/tmpVZBkZa'] returned exit > code 2 > Traceback (most recent call last): > File "../../testing/scripts/run_telemetry_as_googletest.py", line 72, in > <module> > sys.exit(main()) > File "../../testing/scripts/run_telemetry_as_googletest.py", line 44, in main > results = json.load(f) > File "/usr/lib/python2.7/json/__init__.py", line 278, in load > **kw) > File "/usr/lib/python2.7/json/__init__.py", line 326, in loads > return _default_decoder.decode(s) > File "/usr/lib/python2.7/json/decoder.py", line 366, in decode > obj, end = self.raw_decode(s, idx=_w(s, 0).end()) > File "/usr/lib/python2.7/json/decoder.py", line 384, in raw_decode > raise ValueError("No JSON object could be decoded") > ValueError: No JSON object could be decoded > > I think this could be because telemetry's run_tests.py requires the presence of > the browser, and we don't hook up declare browser as dependency for > telemetry_gpu_unittests in BUILD.gn. Filed crbug.com/534585 telemetry_gpu_unittests doesn't require a browser, so if Telemetry's run_tests.py requires one, I agree that is a problem that needs to be fixed. I'm mystified how the recipe ran successfully on your machine if the telemetry_gpu_unittests isolate isn't working because it doesn't contain a browser.
On 2015/09/21 22:27:15, Ken Russell wrote: > On 2015/09/21 21:25:38, nednguyen wrote: > > On 2015/09/21 20:59:39, Ken Russell wrote: > > > > > > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage.py > > > File testing/buildbot/manage.py (right): > > > > > > > > > https://codereview.chromium.org/1354223004/diff/60001/testing/buildbot/manage... > > > testing/buildbot/manage.py:75: 'telemetry_gpu_unittests', > > > On 2015/09/21 17:29:12, nednguyen wrote: > > > > On 2015/09/21 17:26:18, nednguyen wrote: > > > > > On 2015/09/18 23:52:51, Ken Russell wrote: > > > > > > I don't think this is needed. Could you please take it out and re-send > > the > > > > CQ > > > > > > dry run? > > > > > > > > > > Done. > > > > > > > > Actually, this is needed to bypass the presubmit check. Without this, I > got > > > the > > > > complaint: > > > > > > > > ** Presubmit ERRORS ** > > > > manage (0.08s) failed > > > > telemetry_gpu_unittests is listed in gn_isolate_map.pyl but not in any > .json > > > > files > > > > > > > > > > Pawel, Dirk, do either of you know why that's happening? Does this new test > > need > > > to be added elsewhere to the .json files? I don't see any place where build > > > targets are listed separately from targets that would be run. > > > > Ken, after the fix in infra/scripts/runtest_wrapper.py, the log is showing a > > different failure: > > > > Running ['/usr/bin/python', '../../content/test/gpu/run_unittests.py', '-v', > > '--write-full-results-to', '/tmp/isolated_tmpbO4FrN/tmpVZBkZa'] > > libdc1394 error: Failed to initialize libdc1394 > > Usage: run_tests.py [test_name ...] [<options>] > > > > run_tests.py: error: No browser found of type any. Cannot run tests. > > Re-run with --browser=list to see available browser types. > > Command ['/usr/bin/python', '../../content/test/gpu/run_unittests.py', '-v', > > '--write-full-results-to', '/tmp/isolated_tmpbO4FrN/tmpVZBkZa'] returned exit > > code 2 > > Traceback (most recent call last): > > File "../../testing/scripts/run_telemetry_as_googletest.py", line 72, in > > <module> > > sys.exit(main()) > > File "../../testing/scripts/run_telemetry_as_googletest.py", line 44, in > main > > results = json.load(f) > > File "/usr/lib/python2.7/json/__init__.py", line 278, in load > > **kw) > > File "/usr/lib/python2.7/json/__init__.py", line 326, in loads > > return _default_decoder.decode(s) > > File "/usr/lib/python2.7/json/decoder.py", line 366, in decode > > obj, end = self.raw_decode(s, idx=_w(s, 0).end()) > > File "/usr/lib/python2.7/json/decoder.py", line 384, in raw_decode > > raise ValueError("No JSON object could be decoded") > > ValueError: No JSON object could be decoded > > > > I think this could be because telemetry's run_tests.py requires the presence > of > > the browser, and we don't hook up declare browser as dependency for > > telemetry_gpu_unittests in BUILD.gn. Filed crbug.com/534585 > > telemetry_gpu_unittests doesn't require a browser, so if Telemetry's > run_tests.py requires one, I agree that is a problem that needs to be fixed. > > I'm mystified how the recipe ran successfully on your machine if the > telemetry_gpu_unittests isolate isn't working because it doesn't contain a > browser. The last tryjob run succeeds (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...), this patch is ready for review.
Still LGTM with caveat. https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:658: data_deps = [ "//chrome" ] Thanks for tracking down this problem. I don't understand how this isolate worked locally. Let's fix crbug.com/534585 quickly so that this dependency can be removed; otherwise, the isolated telemetry_gpu_unittests won't work on any non-GN platform (since telemetry_gpu_unittests.isolate and the GYP dependencies aren't being updated). Understanding this issue now, perhaps it does make sense to fix crbug.com/534585 first.
https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:658: data_deps = [ "//chrome" ] On 2015/09/21 23:04:37, Ken Russell wrote: > Thanks for tracking down this problem. I don't understand how this isolate > worked locally. Let's fix crbug.com/534585 quickly so that this dependency can > be removed; otherwise, the isolated telemetry_gpu_unittests won't work on any > non-GN platform (since telemetry_gpu_unittests.isolate and the GYP dependencies > aren't being updated). Which are the non-GN platforms? > Understanding this issue now, perhaps it does make sense > to fix crbug.com/534585 first. Acknowledged. Though I think we still don't need to block this patch on fixing crbug.com/534585. Since your tests in telemetry_gpu_unittests don't touch the actual browser, it doesn't matter much which platform we use for running it and the test coverage should be the same.
On 2015/09/21 23:13:01, nednguyen wrote: > https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn > File chrome/test/BUILD.gn (right): > > https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn#n... > chrome/test/BUILD.gn:658: data_deps = [ "//chrome" ] > On 2015/09/21 23:04:37, Ken Russell wrote: > > Thanks for tracking down this problem. I don't understand how this isolate > > worked locally. Let's fix crbug.com/534585 quickly so that this dependency can > > be removed; otherwise, the isolated telemetry_gpu_unittests won't work on any > > non-GN platform (since telemetry_gpu_unittests.isolate and the GYP > dependencies > > aren't being updated). > > Which are the non-GN platforms? Everything except Linux at the moment. > > Understanding this issue now, perhaps it does make sense > > to fix crbug.com/534585 first. > > Acknowledged. > > > Though I think we still don't need to block this patch on fixing > crbug.com/534585. Since your tests in telemetry_gpu_unittests don't touch the > actual browser, it doesn't matter much which platform we use for running it and > the test coverage should be the same. OK, as long as we get to crbug.com/534585 quickly. LGTM
nednguyen@google.com changed reviewers: + phajdan.jr@chromium.org
Somwhat rubberstamp lgtm https://codereview.chromium.org/1354223004/diff/180001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1354223004/diff/180001/tools/mb/mb.py#newcode561 tools/mb/mb.py:561: ] + gn_isolate_map[target].get('args', []) It'd be more readable to use: ] cmdline.extend(gn_isolate_map[target].get('args', []))
lgtm https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:658: data_deps = [ "//chrome" ] On 2015/09/21 23:13:01, nednguyen wrote: > On 2015/09/21 23:04:37, Ken Russell wrote: > > Thanks for tracking down this problem. I don't understand how this isolate > > worked locally. Let's fix crbug.com/534585 quickly so that this dependency can > > be removed; otherwise, the isolated telemetry_gpu_unittests won't work on any > > non-GN platform (since telemetry_gpu_unittests.isolate and the GYP > dependencies > > aren't being updated). > > Which are the non-GN platforms? > Most of them :). Only the main linux release and debug bots have been flipped to GN at this point. https://codereview.chromium.org/1354223004/diff/180001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/1354223004/diff/180001/tools/mb/mb.py#newcode561 tools/mb/mb.py:561: ] + gn_isolate_map[target].get('args', []) On 2015/09/22 17:08:19, M-A Ruel wrote: > It'd be more readable to use: > ] > cmdline.extend(gn_isolate_map[target].get('args', [])) meh :)
On 2015/09/22 17:48:53, Dirk Pranke wrote: > lgtm > > https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn > File chrome/test/BUILD.gn (right): > > https://codereview.chromium.org/1354223004/diff/180001/chrome/test/BUILD.gn#n... > chrome/test/BUILD.gn:658: data_deps = [ "//chrome" ] > On 2015/09/21 23:13:01, nednguyen wrote: > > On 2015/09/21 23:04:37, Ken Russell wrote: > > > Thanks for tracking down this problem. I don't understand how this isolate > > > worked locally. Let's fix crbug.com/534585 quickly so that this dependency > can > > > be removed; otherwise, the isolated telemetry_gpu_unittests won't work on > any > > > non-GN platform (since telemetry_gpu_unittests.isolate and the GYP > > dependencies > > > aren't being updated). > > > > Which are the non-GN platforms? > > > > Most of them :). Only the main linux release and debug bots have been flipped to > GN at this point. > > https://codereview.chromium.org/1354223004/diff/180001/tools/mb/mb.py > File tools/mb/mb.py (right): > > https://codereview.chromium.org/1354223004/diff/180001/tools/mb/mb.py#newcode561 > tools/mb/mb.py:561: ] + gn_isolate_map[target].get('args', []) > On 2015/09/22 17:08:19, M-A Ruel wrote: > > It'd be more readable to use: > > ] > > cmdline.extend(gn_isolate_map[target].get('args', [])) > > meh :) @Pawel: ping
https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... File infra/scripts/runtest_wrapper.py (right): https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... infra/scripts/runtest_wrapper.py:29: if runtest_args[0] == '--': Wait, why is this logic needed? I wouldn't mind too much if this was some code specific to your test, but this wrapper would live at least for some time (and I have further cleanup plans related to it). I might be able to suggest something here or even fix things myself - for now however, I'd just like to understand what you're trying to do.
https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... File infra/scripts/runtest_wrapper.py (right): https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... infra/scripts/runtest_wrapper.py:29: if runtest_args[0] == '--': On 2015/09/24 14:29:22, Paweł Hajdan Jr. wrote: > Wait, why is this logic needed? > > I wouldn't mind too much if this was some code specific to your test, but this > wrapper would live at least for some time (and I have further cleanup plans > related to it). > > I might be able to suggest something here or even fix things myself - for now > however, I'd just like to understand what you're trying to do. To use the isolate_script_test, we need to add the extra command args "-- --isolated-script-test-output" to the arguments list passed to runtest_wrapper.py. With the original logic, nargs='*' will skip all the "--" in the command line arguments. (see the log in http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Therefore I need to modify parser.add_argument('args', nargs='*'..) to parser.add_argument('args', nargs=argparse.REMAINDER..). However, the argument we pass to runtest_wrapper is: "--path-build /b/build -- --target Release --xvfb...", which means using argparse.REMAINDER keep the "--" right before "--isolated-script-test-output", but also the one right before "--target". Hence I need to strip out the starting "--" here.
On 2015/09/24 15:54:54, nednguyen wrote: > https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... > File infra/scripts/runtest_wrapper.py (right): > > https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... > infra/scripts/runtest_wrapper.py:29: if runtest_args[0] == '--': > On 2015/09/24 14:29:22, Paweł Hajdan Jr. wrote: > > Wait, why is this logic needed? > > > > I wouldn't mind too much if this was some code specific to your test, but this > > wrapper would live at least for some time (and I have further cleanup plans > > related to it). > > > > I might be able to suggest something here or even fix things myself - for now > > however, I'd just like to understand what you're trying to do. > > To use the isolate_script_test, we need to add the extra command args "-- > --isolated-script-test-output" to the arguments list passed to > runtest_wrapper.py. > > With the original logic, nargs='*' will skip all the "--" in the command line > arguments. (see the log in > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Therefore I need to modify parser.add_argument('args', nargs='*'..) to > parser.add_argument('args', nargs=argparse.REMAINDER..). > > However, the argument we pass to runtest_wrapper is: > "--path-build /b/build -- --target Release --xvfb...", which means using > argparse.REMAINDER keep the "--" right before "--isolated-script-test-output", > but also the one right before "--target". > > Hence I need to strip out the starting "--" here. Ping Pawel.
On 2015/09/25 23:13:57, nednguyen wrote: > On 2015/09/24 15:54:54, nednguyen wrote: > > > https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... > > File infra/scripts/runtest_wrapper.py (right): > > > > > https://codereview.chromium.org/1354223004/diff/180001/infra/scripts/runtest_... > > infra/scripts/runtest_wrapper.py:29: if runtest_args[0] == '--': > > On 2015/09/24 14:29:22, Paweł Hajdan Jr. wrote: > > > Wait, why is this logic needed? > > > > > > I wouldn't mind too much if this was some code specific to your test, but > this > > > wrapper would live at least for some time (and I have further cleanup plans > > > related to it). > > > > > > I might be able to suggest something here or even fix things myself - for > now > > > however, I'd just like to understand what you're trying to do. > > > > To use the isolate_script_test, we need to add the extra command args "-- > > --isolated-script-test-output" to the arguments list passed to > > runtest_wrapper.py. > > > > With the original logic, nargs='*' will skip all the "--" in the command line > > arguments. (see the log in > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > Therefore I need to modify parser.add_argument('args', nargs='*'..) to > > parser.add_argument('args', nargs=argparse.REMAINDER..). > > > > However, the argument we pass to runtest_wrapper is: > > "--path-build /b/build -- --target Release --xvfb...", which means using > > argparse.REMAINDER keep the "--" right before "--isolated-script-test-output", > > but also the one right before "--target". > > > > Hence I need to strip out the starting "--" here. > > Ping Pawel. Ping
I'm working on an IMO improved version of this that'd address my concerns, see e.g. https://codereview.chromium.org/1374943002 .
On 2015/09/29 15:06:43, Paweł Hajdan Jr. wrote: > Also see https://codereview.chromium.org/1378623002 Thanks Pawel. In the meantime we'll try to get rid of the dependency on the browser in the telemetry_gpu_unittests step.
On 2015/09/29 17:24:47, Ken Russell wrote: > On 2015/09/29 15:06:43, Paweł Hajdan Jr. wrote: > > Also see https://codereview.chromium.org/1378623002 > > Thanks Pawel. In the meantime we'll try to get rid of the dependency on the > browser in the telemetry_gpu_unittests step. @Pawel, please take a look again.
Thanks Ned for getting rid of the "chrome" dependency in telemetry_gpu_unittests. This looks great to me. Pawel, would you please re-review this? lgtm
Patch 12 LGTM.
The CQ bit was checked by nednguyen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/1354223004/#ps220001 (title: "Add third_party/webgl/src/sdk/tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354223004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354223004/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/fbee71a6424ca43fbf8b87add04f132a83abe64c Cr-Commit-Position: refs/heads/master@{#351784} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
