+eyaich/ashleymarie

There doesn't appear to be an upload step, and the buildbot page says this test is disabled, although the output suggests it ran just fine.

Ashley, could this be related to https://chromium-review.googlesource.com/c/chromium/tools/build/+/646566?

This was the first build that appeared to have the problem:

https://build.chromium.org/p/chromium.perf/builders/Win%207%20Nvidia%20GPU%20Perf/builds/1239

Last successful build:

https://build.chromium.org/p/chromium.perf/builders/Win%207%20Nvidia%20GPU%20Perf/builds/1238

Looks like angle_perftests are still attempting to generate the simplified json instead of json-test-results format. Lemme do some digging to figure out why that flag wasn't flipped for these tests. If this is urgent, we could revert the change linked in Comment 1 since that change started checking the json-test-results format instead of chartjson to determine if a test was disabled or not.

Ah I get it, run_telemetry_benchmark_as_googletest.py isn't used to run angle_perftests or probably the other non-telemetry benchmarks (net_perftests, cc_perftests, gpu_perftests, tracing_perftests, load_library_perf_tests, media_perftests, performance_browser_tests). So none of those tests are generating the new json-test-results output format.

Thanks for figuring this out Ashley. It would be good to fix it since these tests are the main thing keeping performance regressions from landing in our project. If it's something that might take longer than a day or so I'd say best to revert, otherwise no problem, can just fix it and move forward.

My change was blocking eakeufner@'s change from chartjson to histogram set so it would be difficult to revert my change directly

I think the long term solution here would be update https://cs.chromium.org/chromium/src/testing/scripts/run_gtest_perf_test.py?l=128 to output the correct json-test-results format but that is likely going to take longer than a day to sort through

A short term solution would be to update https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_tests/steps.py?l=1639 to be something like "do json-test-results exist? yes: utilize those to determine if the tests are enabled; no: utilize chart json results to determine if the tests are enabled. Once the tests are outputing json-test-results instead of simplified json, we could revert this change

Thoughts?

Short term fix available here: https://chromium-review.googlesource.com/c/chromium/tools/build/+/663787

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/e09eeffbfbb9968253959f4f49b82b25b623bd48

commit e09eeffbfbb9968253959f4f49b82b25b623bd48
Author: Ashley Enstad <ashleymarie@chromium.org>
Date: Tue Sep 12 21:23:11 2017

If no json-test-results are generated, utilize the chartjson as a backup

Can revert this change once non-telemetry benchmarks are outputing the
new json-test-results format instead of the old simplified json format.

BUG= chromium:764350 

Change-Id: Ia5abb56839d5db5fec73552f83268b91072bf2db
Reviewed-on: https://chromium-review.googlesource.com/663787
Commit-Queue: Ashley Enstad <ashleymarie@chromium.org>
Reviewed-by: Ethan Kuefner <eakuefner@chromium.org>
Reviewed-by: Stephen Martinis <martiniss@chromium.org>

[modify] https://crrev.com/e09eeffbfbb9968253959f4f49b82b25b623bd48/scripts/slave/README.recipes.md
[modify] https://crrev.com/e09eeffbfbb9968253959f4f49b82b25b623bd48/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.py
[modify] https://crrev.com/e09eeffbfbb9968253959f4f49b82b25b623bd48/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.expected/chartjson_simplified_ignore_task_failure.json
[add] https://crrev.com/e09eeffbfbb9968253959f4f49b82b25b623bd48/scripts/slave/recipe_modules/chromium_tests/tests/steps/swarming_isolated_script_test.expected/chartjson_simplified_disabled.json
[modify] https://crrev.com/e09eeffbfbb9968253959f4f49b82b25b623bd48/scripts/slave/recipe_modules/chromium_tests/steps.py

Short term fix sounds great, appreciate it. Lowering priority as the immediate need is resolved.

Looks like the tests are back to uploading and I see data from 9/13 here: https://chromeperf.appspot.com/report?sid=54496989934444723c40d6729afc789bb977b0f9db9863db22e318c6bbfe15c9&rev=501473

I've got a follow-up bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=764501
which tracks the issue of getting these tests to output the new json format.

So I think this bug should be able to be marked fixed
Feel free to reopen if you don't consider it fixed :)

Nope, looks great. Thank you!