Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1922163002:
Initial support to test.dart for running precompiler tests on android devices (Closed)
|
Created:
4 years, 8 months ago by kustermann Modified:
4 years, 7 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionInitial support to test.dart for running precompiler tests on android devices
Steps to make this work:
a) Make sure you have an android sdk checkout:
=> See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android
b) Building using android-toolchain crosscompiler:
$ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime
=> Notice --os=android
c) Testing using attached android phones:
$ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools
$ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs
=> Notice --system=android
Failing tests on android can be marked in status files via:
[ $compiler == precompiler && $runtime == dart_precompiled && $system == android ]
R=whesse@google.com
Committed: https://github.com/dart-lang/sdk/commit/620b896e2f38b618485a420cabf0c2ea97154593
Patch Set 1 #
Total comments: 4
Patch Set 2 : Some fixes #
Total comments: 28
Patch Set 3 : #Patch Set 4 : Cleanup of CL and of existing code. #
Total comments: 10
Patch Set 5 : #
Messages
Total messages: 22 (7 generated)
Description was changed from ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android ========== to ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android Failing tests on android can be marked in status files via: [ $runtime == dart_precompiled && $system == android ] ==========
Description was changed from ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android Failing tests on android can be marked in status files via: [ $runtime == dart_precompiled && $system == android ] ========== to ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android Failing tests on android can be marked in status files via: [ $runtime == dart_precompiled && $system == android ] ==========
Description was changed from ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android Failing tests on android can be marked in status files via: [ $runtime == dart_precompiled && $system == android ] ========== to ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android Failing tests on android can be marked in status files via: [ $compiler == precompiler && $runtime == dart_precompiled && $system == android ] ==========
kustermann@google.com changed reviewers: + fschneider@google.com, whesse@google.com
rmacnak@google.com changed reviewers: + rmacnak@google.com
https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/compiler... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/compiler... tools/testing/dart/compiler_configuration.dart:348: args.addAll(arguments); if (targeting 32 bit ARM Android) { args.add("--no-sim-use-hardfp"); }
https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/android.... File tools/testing/dart/android.dart (right): https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/android.... tools/testing/dart/android.dart:326: var args = ['shell', 'sh', '-c', For some reason I had to remove "sh -c" and the single-quotes in the line below to make it work on my machine: - var args = ['shell', 'sh', '-c', - "'${shellArgs.join(' ')} ; echo $MARKER \$?'"]; + var args = ['shell', + "${shellArgs.join(' ')} ; echo $MARKER \$?"]; +
https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/android.... File tools/testing/dart/android.dart (right): https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/android.... tools/testing/dart/android.dart:326: var args = ['shell', 'sh', '-c', On 2016/04/27 09:10:37, Florian Schneider wrote: > For some reason I had to remove "sh -c" and the single-quotes in the line below > to make it work on my machine: > > - var args = ['shell', 'sh', '-c', > - "'${shellArgs.join(' ')} ; echo $MARKER \$?'"]; > + var args = ['shell', > + "${shellArgs.join(' ')} ; echo $MARKER \$?"]; > + Both seem to work for me, so I changed it. https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/compiler... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/1/tools/testing/dart/compiler... tools/testing/dart/compiler_configuration.dart:348: args.addAll(arguments); On 2016/04/26 17:18:07, rmacnak wrote: > if (targeting 32 bit ARM Android) { > args.add("--no-sim-use-hardfp"); > } Good catch, thanks.
https://codereview.chromium.org/1922163002/diff/20001/runtime/bin/log_android.cc File runtime/bin/log_android.cc (right): https://codereview.chromium.org/1922163002/diff/20001/runtime/bin/log_android... runtime/bin/log_android.cc:19: void Log::VPrint(const char* format, va_list args) { Add a comment explaining this https://codereview.chromium.org/1922163002/diff/20001/tools/test.dart File tools/test.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/test.dart#newcode64 tools/test.dart:64: await testConfigurations(configurations); What does this even do? We aren't doing anything with the returned future. Couldn't this just be replaced with "return testConfigurations(configurations);", since .then handles a returned future? https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... File tools/testing/dart/android.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:31: Future<String> _executeCommandGetOutput(String executable, List<String> args, Do we really need two functions, especially since these are private? Just one returning Future<AdbCommandResult> should be fine. Getting the string is simple, just .stdout. Also, If you make the throwIfFailed a method on AdbCommandResult: return (await _executeCommandRaw(...))..throwIfFailed(); https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:74: ]; I think this is much simpler with async-await. We don't even need to run in parallel, but we can: var stdput = getOutput(process.stdout); .. return new AdbCommandResult(command, await stdout, await stderr, await process.exitCode); https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:344: Future _adbCommand(List<String> adbArgs) { These two could also be united to one that returns Future<AdbCommandResult>. Then the factoring of deviceSpecificArgs isn't as necessary. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:396: * Discovers all available devices and supports aquire/release. acquire https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:415: Future<AdbDevice> aquireDevice() async { acquire https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:427: Completer completer = _waiter.removeLast(); Both idle devices and waiting requests are LIFO. This seems worse than FIFO. Using queues rather than list seems worth it. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:350: args.add('--no-sim-use-hardfp'); I think this should be before the arguments parameter is added. If the input file is last, it should remain last. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:389: cc_flags = ""; Maybe use null here instead. Or find out the correct -m flag to use, if there is one. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:398: if (cc_flags != null && cc_flags.length > 0) args.add(cc_flags); Decide which value to use for "none" in arch == 'arm' case, and only check that. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/runt... File tools/testing/dart/runtime_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/runt... tools/testing/dart/runtime_configuration.dart:301: artifact.filename, script is a local == artifact.filename. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/test... File tools/testing/dart/test_configurations.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/test... tools/testing/dart/test_configurations.dart:294: } We have code for awaiting the AdbDevices in browser_controller.dart: var idbNames = await AdbHelper.listDevices(); idleAdbDevices = new List.from(idbNames.map((id) => new AdbDevice(id))); That should perhaps use your pool, or maybe the pool could be moved down to a lower level than ProcessQueue, and initialized later. If it has to be up at this level, we could wait for it before the startProcessQueue function, down where we wait for serverFutures, and they all could be awaits. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/test... tools/testing/dart/test_configurations.dart:316: await Future.wait(serverFutures).then((_) => startProcessQueue()); This should just be: if (serverFutures.isNotEmpty) { await Future.wait(serverFutures); } await startProcessQueue(); Or: var pool = await AdbDevicePoolIfNeeded; startProcessQueue(pool);
PTAL https://codereview.chromium.org/1922163002/diff/20001/runtime/bin/log_android.cc File runtime/bin/log_android.cc (right): https://codereview.chromium.org/1922163002/diff/20001/runtime/bin/log_android... runtime/bin/log_android.cc:19: void Log::VPrint(const char* format, va_list args) { On 2016/04/27 12:22:31, Bill Hesse wrote: > Add a comment explaining this Done. https://codereview.chromium.org/1922163002/diff/20001/tools/test.dart File tools/test.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/test.dart#newcode64 tools/test.dart:64: await testConfigurations(configurations); > What does this even do? We aren't doing anything with the returned future. It runs all the tests. So it makes sense that it returns a future. > Couldn't this just be replaced with "return > testConfigurations(configurations);", since .then handles a returned future? I think it makes it clearer that we wait until testing all configurations is done. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... File tools/testing/dart/android.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:31: Future<String> _executeCommandGetOutput(String executable, List<String> args, On 2016/04/27 12:22:31, Bill Hesse wrote: > Do we really need two functions, especially since these are private? > Just one returning Future<AdbCommandResult> should be fine. Getting the string > is simple, just .stdout. > > > Also, > If you make the throwIfFailed a method on AdbCommandResult: > > return (await _executeCommandRaw(...))..throwIfFailed(); Done. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:74: ]; On 2016/04/27 12:22:31, Bill Hesse wrote: > I think this is much simpler with async-await. > We don't even need to run in parallel, but we can: > var stdput = getOutput(process.stdout); > .. > > return new AdbCommandResult(command, await stdout, await stderr, await > process.exitCode); If we don't drain stdout/stderr, then the buffer runs full and it can lead to a deadlock. We need therefore to drain them in parallel. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:344: Future _adbCommand(List<String> adbArgs) { On 2016/04/27 12:22:31, Bill Hesse wrote: > These two could also be united to one that returns Future<AdbCommandResult>. > Then the factoring of deviceSpecificArgs isn't as necessary. Done. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:396: * Discovers all available devices and supports aquire/release. On 2016/04/27 12:22:31, Bill Hesse wrote: > acquire Done. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:415: Future<AdbDevice> aquireDevice() async { On 2016/04/27 12:22:31, Bill Hesse wrote: > acquire Done. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:427: Completer completer = _waiter.removeLast(); On 2016/04/27 12:22:31, Bill Hesse wrote: > Both idle devices and waiting requests are LIFO. This seems worse than FIFO. > Using queues rather than list seems worth it. Fair enough. Though we have just about 4 devices and we don't care about LIFO vs FIFO. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:350: args.add('--no-sim-use-hardfp'); On 2016/04/27 12:22:32, Bill Hesse wrote: > I think this should be before the arguments parameter is added. If the input > file is last, it should remain last. It is in the latest patchset. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:389: cc_flags = ""; On 2016/04/27 12:22:32, Bill Hesse wrote: > Maybe use null here instead. Or find out the correct -m flag to use, if there > is one. Done. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:398: if (cc_flags != null && cc_flags.length > 0) args.add(cc_flags); On 2016/04/27 12:22:31, Bill Hesse wrote: > Decide which value to use for "none" in arch == 'arm' case, and only check that. Done. https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/runt... File tools/testing/dart/runtime_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/runt... tools/testing/dart/runtime_configuration.dart:301: artifact.filename, On 2016/04/27 12:22:32, Bill Hesse wrote: > script is a local == artifact.filename. Done. Strictly speaking it's not even a filename but a directory name :-/ https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/test... File tools/testing/dart/test_configurations.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/test... tools/testing/dart/test_configurations.dart:294: } On 2016/04/27 12:22:32, Bill Hesse wrote: > We have code for awaiting the AdbDevices in browser_controller.dart: > > var idbNames = await AdbHelper.listDevices(); > idleAdbDevices = new List.from(idbNames.map((id) => new AdbDevice(id))); > > That should perhaps use your pool, or maybe the pool could be moved down to a > lower level than ProcessQueue, and initialized later. > > If it has to be up at this level, we could wait for it before the > startProcessQueue function, down where we wait for serverFutures, and they all > could be awaits. I'd really prefer not to modify any unrelated code (especially in browser controller) to avoid breaking anything. There's dead code in browser controller, code that doesn't work, and code that I might not be able to test locally. So I rather not touch it. I also don't like it much and unfortunately all the work is done in the constructor. Refactoring this / cleaning this up should not be part of this CL.
https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/test... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/test... tools/testing/dart/test_runner.dart:2506: return adbDevicePool.aquireDevice().then((AdbDevice device) { s/aquire/acquire/
https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... File tools/testing/dart/android.dart (right): https://codereview.chromium.org/1922163002/diff/20001/tools/testing/dart/andr... tools/testing/dart/android.dart:74: ]; On 2016/04/27 13:25:08, kustermann wrote: > On 2016/04/27 12:22:31, Bill Hesse wrote: > > I think this is much simpler with async-await. > > We don't even need to run in parallel, but we can: > > var stdput = getOutput(process.stdout); > > .. > > > > return new AdbCommandResult(command, await stdout, await stderr, await > > process.exitCode); > > If we don't drain stdout/stderr, then the buffer runs full and it can lead to a > deadlock. We need therefore to drain them in parallel. The draining in parallel does happen, if we do the functions calls, and then just await on variables holding the results. https://codereview.chromium.org/1922163002/diff/60001/tools/test.dart File tools/test.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/test.dart#newcode64 tools/test.dart:64: await testConfigurations(configurations); I still think this is wrong. It generates exactly the same code as before - the async-await do exactly nothing. If we do anything, we should comment that the function does all the testing asynchronously, and returns a future that we don't wait on. As you pointed out, we doubt that the future currently returned from testConfigurations is actually waiting on all async tasks started by all subsystems. We should just comment: // All the testing is carried out asynchronously in tasks created by this call. // TODO(issue number): Ensure that all tasks complete before the returned future completes. Writing await is quite misleading. What we really should do, is make sure everything that does anything is waited on, return the final future to main, and print a message and exit explicitly, and try and find anything that isn't completed at the point we call exit. But https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/andr... File tools/testing/dart/android.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/andr... tools/testing/dart/android.dart:62: String command = '$executable ${args.join(' ')}'; It may be legal to have ' inside ' because it is inside ${}, but I prefer to use " here for one of the pairs of quotes. https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/comp... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:402: var args = (cc_flags != null) ? [ shared, cc_flags ] : [ shared ]; I like this better.
https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/test... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/test... tools/testing/dart/test_runner.dart:2539: ['push', runner, '$devicedir/runner'])); Maybe add a TODO that we should push the runner (dart_precompiled_runtime) only once for all tests, instead of once per test.
lgtm
On 2016/04/28 13:48:40, Bill Hesse wrote:
> lgtm
After running many tests (1000s) I get the following error from test.dart where
it gets stuck - not sure if this is related to this CL though...
--- snip ---
The debug timer of test.dart expired. Please report this issue to ricow/whesse
and provide the following information:
Graph is sealed: true
Count[Initialized] = 0
Count[Enqueuing] = 0
Count[Waiting] = 0
Count[Running] = 1
Count[Failed] = 2938
Count[Successful] = 17353
Count[UnableToRun] = 139
Commands in state 'Running':
=================================
Command: Steps to push precompiled runner and precompiled code to an attached
device. Uses (and requires) adb.
Enqueued by: precompiler-dart_precompiled release_arm --
language/function_type_alias_test
CommandQueue state:
Processes: used: 1 max: 32
BrowserProcesses: used: 0 max: 15
Finishing: false
Queue (length = 0):
On 2016/04/28 15:28:58, Florian Schneider wrote: > On 2016/04/28 13:48:40, Bill Hesse wrote: > > lgtm > > After running many tests (1000s) I get the following error from test.dart where > it gets stuck - not sure if this is related to this CL though... > > > --- snip --- > > The debug timer of test.dart expired. Please report this issue to ricow/whesse > and provide the following information: > > Graph is sealed: true > > Count[Initialized] = 0 > Count[Enqueuing] = 0 > Count[Waiting] = 0 > Count[Running] = 1 > Count[Failed] = 2938 > Count[Successful] = 17353 > Count[UnableToRun] = 139 > > Commands in state 'Running': > ================================= > > Command: Steps to push precompiled runner and precompiled code to an attached > device. Uses (and requires) adb. > Enqueued by: precompiler-dart_precompiled release_arm -- > language/function_type_alias_test > > > > > CommandQueue state: > Processes: used: 1 max: 32 > BrowserProcesses: used: 0 max: 15 > Finishing: false > Queue (length = 0): I noticed one of my phones reboot shortly before seeing the same message. Maybe the adb pipe hangs when this happens?
Patchset #5 (id:80001) has been deleted
Thanks. Let's look at the timeout/adb issue in another CL. https://codereview.chromium.org/1922163002/diff/60001/tools/test.dart File tools/test.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/test.dart#newcode64 tools/test.dart:64: await testConfigurations(configurations); On 2016/04/27 13:54:40, Bill Hesse wrote: > I still think this is wrong. It generates exactly the same code as before - the > async-await do exactly nothing. > > If we do anything, we should comment that the function does all the testing > asynchronously, and returns a future that we don't wait on. > > As you pointed out, we doubt that the future currently returned from > testConfigurations is actually waiting on all async tasks started by all > subsystems. > > We should just comment: > // All the testing is carried out asynchronously in tasks created by this call. > // TODO(issue number): Ensure that all tasks complete before the returned future > completes. > > > > Writing await is quite misleading. What we really should do, is make sure > everything that does anything is waited on, return the final future to main, and > print a message and exit explicitly, and try and find anything that isn't > completed at the point we call exit. But Done - Comment + issue. https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/andr... File tools/testing/dart/android.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/andr... tools/testing/dart/android.dart:62: String command = '$executable ${args.join(' ')}'; On 2016/04/27 13:54:40, Bill Hesse wrote: > It may be legal to have ' inside ' because it is inside ${}, but I prefer to use > " here for one of the pairs of quotes. Done. https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/comp... File tools/testing/dart/compiler_configuration.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/comp... tools/testing/dart/compiler_configuration.dart:402: var args = (cc_flags != null) ? [ shared, cc_flags ] : [ shared ]; On 2016/04/27 13:54:40, Bill Hesse wrote: > I like this better. Acknowledged. https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/test... File tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/test... tools/testing/dart/test_runner.dart:2506: return adbDevicePool.aquireDevice().then((AdbDevice device) { On 2016/04/27 13:53:05, Florian Schneider wrote: > s/aquire/acquire/ Done. https://codereview.chromium.org/1922163002/diff/60001/tools/testing/dart/test... tools/testing/dart/test_runner.dart:2539: ['push', runner, '$devicedir/runner'])); On 2016/04/28 13:18:30, Florian Schneider wrote: > Maybe add a TODO that we should push the runner (dart_precompiled_runtime) only > once for all tests, instead of once per test. Done.
Description was changed from ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android Failing tests on android can be marked in status files via: [ $compiler == precompiler && $runtime == dart_precompiled && $system == android ] ========== to ========== Initial support to test.dart for running precompiler tests on android devices Steps to make this work: a) Make sure you have an android sdk checkout: => See https://github.com/dart-lang/sdk/wiki/Building-Dart-SDK-for-Android b) Building using android-toolchain crosscompiler: $ ./tools/build.py -mrelease -aarm --os=android dart_precompiled_runtime => Notice --os=android c) Testing using attached android phones: $ export PATH=$PATH:$PWD/third_party/android_tools/sdk/platform-tools $ ./tools/test.py -mrelease -aarm --system=android -cprecompiler -rdart_precompiled --use-blobs => Notice --system=android Failing tests on android can be marked in status files via: [ $compiler == precompiler && $runtime == dart_precompiled && $system == android ] R=whesse@google.com Committed: https://github.com/dart-lang/sdk/commit/620b896e2f38b618485a420cabf0c2ea97154593 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as 620b896e2f38b618485a420cabf0c2ea97154593 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
