Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1633713002:
add remaining unittests to new android bots (Closed)
Descriptionadd remaining unittests to new android bots
BUG=514857
Committed: https://crrev.com/3476f7c84c16ed45a8404f491fab9fb4691bda5f
Cr-Commit-Position: refs/heads/master@{#371331}
Patch Set 1 #Patch Set 2 : add isolate file paths #Messages
Total messages: 13 (3 generated)
bpastene@chromium.org changed reviewers: + jbudorick@chromium.org, martiniss@chromium.org
They've all been running green for a while now; I'll add the instrumentation tests later if they're still good.
I think this works, and so in that regard it's ok, but it winds up relying on the default isolate file paths we've hard-coded into the test runner here: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... I've generally been trying to move away from them, but the gtest generator doesn't currently let us pass isolate file paths. Could we potentially modify the generator to accept an isolate file path & pass it here where necessary? Also: - we'll have to either create a generator for instrumentation tests or use the script generator + add another wrapper script in //testing/scripts) - can we nuke https://code.google.com/p/chromium/codesearch#chromium/src/testing/scripts/an... and convert its lone usage to the gtest generator?
On 2016/01/25 18:14:49, jbudorick wrote: > I think this works, and so in that regard it's ok, but it winds up relying on > the default isolate file paths we've hard-coded into the test runner here: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... > > I've generally been trying to move away from them, but the gtest generator > doesn't currently let us pass isolate file paths. Could we potentially modify > the generator to accept an isolate file path & pass it here where necessary? > > Also: > - we'll have to either create a generator for instrumentation tests or use the > script generator + add another wrapper script in //testing/scripts) > - can we nuke > https://code.google.com/p/chromium/codesearch#chromium/src/testing/scripts/an... > and convert its lone usage to the gtest generator? (the last two points would be in separate reviews)
On 2016/01/25 18:14:49, jbudorick wrote: > I think this works, and so in that regard it's ok, but it winds up relying on > the default isolate file paths we've hard-coded into the test runner here: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... > > I've generally been trying to move away from them, but the gtest generator > doesn't currently let us pass isolate file paths. Could we potentially modify > the generator to accept an isolate file path & pass it here where necessary? I don't think we'd have to modify the generator, it has a field for test harness 'args' specifically for this use case. I believe all we'd have to do is add the isolate_path there. (I'd test this out if I could, but no trybots :( > Also: > - we'll have to either create a generator for instrumentation tests or use the > script generator + add another wrapper script in //testing/scripts) > - can we nuke > https://code.google.com/p/chromium/codesearch#chromium/src/testing/scripts/an... > and convert its lone usage to the gtest generator? I'll take care of those other two points in the future; shouldn't be too hard.
lgtm Eventually (tm), I'd like to move these to the generated wrapper scripts (which is what we already use for swarming, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/base/base_unittest...), but this is ok for now. On 2016/01/25 19:36:19, bpastene wrote: > On 2016/01/25 18:14:49, jbudorick wrote: > > I think this works, and so in that regard it's ok, but it winds up relying on > > the default isolate file paths we've hard-coded into the test runner here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... > > > > I've generally been trying to move away from them, but the gtest generator > > doesn't currently let us pass isolate file paths. Could we potentially modify > > the generator to accept an isolate file path & pass it here where necessary? > > I don't think we'd have to modify the generator, it has a field for test harness > 'args' specifically for this use case. I believe all we'd have to do is add the > isolate_path there. (I'd test this out if I could, but no trybots :( oh sweet, I had missed args. > > > Also: > > - we'll have to either create a generator for instrumentation tests or use > the > > script generator + add another wrapper script in //testing/scripts) > > - can we nuke > > > https://code.google.com/p/chromium/codesearch#chromium/src/testing/scripts/an... > > and convert its lone usage to the gtest generator? > > I'll take care of those other two points in the future; shouldn't be too hard.
Filed https://code.google.com/p/chromium/issues/detail?id=581123 for the other things
The CQ bit was checked by bpastene@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1633713002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1633713002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== add remaining unittests to new android bots BUG=514857 ========== to ========== add remaining unittests to new android bots BUG=514857 Committed: https://crrev.com/3476f7c84c16ed45a8404f491fab9fb4691bda5f Cr-Commit-Position: refs/heads/master@{#371331} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3476f7c84c16ed45a8404f491fab9fb4691bda5f Cr-Commit-Position: refs/heads/master@{#371331}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1639603002/ by jbudorick@chromium.org. The reason for reverting is: isolate paths aren't quite right, e.g. https://build.chromium.org/p/chromium.android/builders/Marshmallow%20Tablet%2.... |
