Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935503002:
List Java Instru Test Information From JUnit Runner (Closed)
|
Created:
3 years, 6 months ago by the real yoland Modified:
3 years, 4 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionList Java Instru Test Information From JUnit Runner
This CL allow test information that was once listed by parsing proguard dump or
dexdump to be produced by JUnit runner run all the tests without executing them.
The TestListInstrumentationRunListener is registered in the
BaseChromiumAndroidJUnitRunner and will write all tests to a json file which
then will be pulled to host side by host side runner script
BUG=640116
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2935503002
Cr-Commit-Position: refs/heads/master@{#489826}
Committed: https://chromium.googlesource.com/chromium/src/+/682e23394ce75a8b9d19a5ee351f401706e5cff7
Patch Set 1 #
Total comments: 2
Patch Set 2 : List tests from Android Instrumentation test runner #
Total comments: 10
Patch Set 3 : address comments #
Total comments: 19
Patch Set 4 : address comments #
Total comments: 1
Patch Set 5 : add tests #
Total comments: 18
Patch Set 6 : address comments #Patch Set 7 : compiling error fix #
Total comments: 20
Patch Set 8 : change proguard flag for test annotations + address comments #Patch Set 9 : remove android webview manifest xml #Patch Set 10 : add test exclusion #
Total comments: 4
Patch Set 11 : remove system println #
Total comments: 14
Patch Set 12 : address comments #Patch Set 13 : minor fix #
Total comments: 7
Patch Set 14 : address issues #
Total comments: 7
Patch Set 15 : address nits #Patch Set 16 : Rebase #Patch Set 17 : fix error #Patch Set 18 : change proguard flags for cronets targets #
Total comments: 12
Patch Set 19 : address nyquist comments #Dependent Patchsets: Messages
Total messages: 126 (86 generated)
yolandyan@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
please let me know about your concerns for this CL As for example, please checkout https://codereview.chromium.org/2935503002/#ps1
On 2017/06/09 at 21:38:30, the real yoland wrote: > please let me know about your concerns for this CL > > As for example, please checkout https://codereview.chromium.org/2935503002/#ps1 *example cl: https://codereview.chromium.org/2933623002/#ps1
I think I may have misunderstood your strategy for converting the WebView tests. The level of webview specificity in here is concerning. https://codereview.chromium.org/2935503002/diff/1/build/android/pylib/instrum... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:242: if t['is_junit4'] and t.get('webview_parameterization'): I'm not sure why this is necessary beyond what GetUniqueTestName already does. https://codereview.chromium.org/2935503002/diff/1/build/android/pylib/instrum... build/android/pylib/instrumentation/instrumentation_test_instance.py:893: if (_WEBVIEW_JUNIT4_PARAMETERIZATION in t['annotations'] This is too webview-specific. Let's talk about generalizing it.
Description was changed from ========== Python Support WebView JUnit4 instrumentation test parameterization This CL add a mechanism which looks for JUnit4 with @WebViewParameterization and copy the test to a new one with the postfix "__sanboxed_mode". This CL also changed the test filter mechanism to return the test name without postfix if it was the copied parameterized test, this is for the purpose of allow filter to match both parameterized and non-parameterized tests. BUG=640116 ========== to ========== List Java Instru Test Information From JUnit Runner This CL allow test information that was once listed by parsing proguard dump or dexdump to be produced by JUnit runner run all the tests without executing them. The TestListInstrumentationRunListener is registered in the BaseChromiumAndroidJUnitRunner and will write all tests to a json file which then will be pulled to host side by host side runner script BUG=640116 ==========
The process takes about 10 seconds to list all tests for Chrome public test apk.
https://codereview.chromium.org/2935503002/diff/20001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java (right): https://codereview.chromium.org/2935503002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:57: results = executorBuilder.build().execute(listTestRequest); can you explain how this works? How did you make the tests not actually get run? https://codereview.chromium.org/2935503002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:58: } catch (RuntimeException e) { Should we actually be catching this? Will the exception show up in the python output? https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:369: if test_runner_junit4: Question. Where are junit3 test listed out? I dont see it. https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:376: target, extras=extras, timeout=10000, retries=0) isnt timeout in seconds? 10000 seems intense! https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:923: def ReadAndSetTests(self, host_file): Is this used?
https://codereview.chromium.org/2935503002/diff/20001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java (right): https://codereview.chromium.org/2935503002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:57: results = executorBuilder.build().execute(listTestRequest); On 2017/07/11 at 21:14:51, mikecase wrote: > can you explain how this works? How did you make the tests not actually get run? I am using the AndroidJUnitRunner's provided feature to log out test only (`-e log true`) https://codereview.chromium.org/2935503002/diff/20001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:58: } catch (RuntimeException e) { On 2017/07/11 at 21:14:51, mikecase wrote: > Should we actually be catching this? Will the exception show up in the python output? The failure stack trace will show up in the host side as output, and the host script will raise Exception https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:369: if test_runner_junit4: On 2017/07/11 at 21:14:51, mikecase wrote: > Question. Where are junit3 test listed out? I dont see it. JUnit3 tests are now listed together with JUnit4 tests in the same run https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:376: target, extras=extras, timeout=10000, retries=0) On 2017/07/11 at 21:14:51, mikecase wrote: > isnt timeout in seconds? 10000 seems intense! Done https://codereview.chromium.org/2935503002/diff/20001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:923: def ReadAndSetTests(self, host_file): On 2017/07/11 at 21:14:51, mikecase wrote: > Is this used? My bad, removed
John, the approach I told you earlier was to use command line argument `-e runListener org.chromium.base.test.TestListInstrumentationRunListener", however I realized that this would not work when `-e log true` is specified since based on the source code (/support/src/android/support/test/runner/AndroidJUnitRunner.java), the runner ignores all other runlistener when `-e log true` is specified So only this would work :( This is only for when JUnit3 is around though. In a JUnit4 only world, this world can be done through our class runner
https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java (right): https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:12: import android.support.test.internal.runner.RunnerArgs; Talking about this offline. https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:30: private static final String LIST_ALL_TESTS_FLAG = "listAllTests"; flags should include the class name, e.g. https://codesearch.chromium.org/chromium/src/testing/android/native_test/java... https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:58: Bundle results = new Bundle(); nit: extract the body of this if block to a separate function. https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:88: return arguments != null ? arguments.getCharSequence(LIST_ALL_TESTS_FLAG) : null; Why getCharSequence and not getString? https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:79: PathUtils.getExternalStorageDirectory() + "/chromium_tests_root/" + mOutputPath); This should be provided with an absolute path, not a relative path in the known external storage directory. https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:192: GetTestNameWithoutParameterPostfix(t, sep='.'), These make it so that, if someone passes the test name with the parameter postfix, we run both? I'm not sure we ought to do that. https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:254: tests = _GetAllTestsFromRunner(device, test_package, filename, What's going on here? Why does it get the tests from the runner, then from the pickle, then if fails to get tests from the pickle, get them from the runner again? https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:366: device_path = os.path.join(device.GetExternalStoragePath(), just use a device temp file: https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil... https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:371: if test_runner_junit4: If there is no test_runner_junit4, it looks like we do nothing and then pulling the file fails below. We should instead either: - do something; or - raise an exception https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:378: target, extras=extras, timeout=30, retries=0)) No retries?
https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java (right): https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:30: private static final String LIST_ALL_TESTS_FLAG = "listAllTests"; On 2017/07/14 at 18:43:55, jbudorick wrote: > flags should include the class name, e.g. https://codesearch.chromium.org/chromium/src/testing/android/native_test/java... Done https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:58: Bundle results = new Bundle(); On 2017/07/14 at 18:43:55, jbudorick wrote: > nit: extract the body of this if block to a separate function. Done https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:88: return arguments != null ? arguments.getCharSequence(LIST_ALL_TESTS_FLAG) : null; On 2017/07/14 at 18:43:55, jbudorick wrote: > Why getCharSequence and not getString? Done https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/40001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:79: PathUtils.getExternalStorageDirectory() + "/chromium_tests_root/" + mOutputPath); On 2017/07/14 at 18:43:55, jbudorick wrote: > This should be provided with an absolute path, not a relative path in the known external storage directory. Done https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:192: GetTestNameWithoutParameterPostfix(t, sep='.'), On 2017/07/14 at 18:43:55, jbudorick wrote: > These make it so that, if someone passes the test name with the parameter postfix, we run both? I'm not sure we ought to do that. I think this just makes that if someone pass in test name without parameter postfix, we run both. If the -f argument has parameter postfix, it will only match the parameterized test. https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:254: tests = _GetAllTestsFromRunner(device, test_package, filename, On 2017/07/14 at 18:43:55, jbudorick wrote: > What's going on here? Why does it get the tests from the runner, then from the pickle, then if fails to get tests from the pickle, get them from the runner again? oops, my bad Done https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:366: device_path = os.path.join(device.GetExternalStoragePath(), On 2017/07/14 at 18:43:55, jbudorick wrote: > just use a device temp file: https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil... Done https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:371: if test_runner_junit4: On 2017/07/14 at 18:43:56, jbudorick wrote: > If there is no test_runner_junit4, it looks like we do nothing and then pulling the file fails below. We should instead either: > - do something; or > - raise an exception Done https://codereview.chromium.org/2935503002/diff/40001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:378: target, extras=extras, timeout=30, retries=0)) On 2017/07/14 at 18:43:55, jbudorick wrote: > No retries? Set retries to 2? Done
Added Java robolectric tests (because of dep on org.json.JSONObject) and python instrumentation test instance tests
The CQ bit was checked by yolandyan@chromium.org 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
some nits. But this looks pretty good to me. https://codereview.chromium.org/2935503002/diff/60001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/60001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:152: return annotaionsJsons; annotaionsJsons https://codereview.chromium.org/2935503002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:126: * or "hashCode". Smallest nit of all time: "equal()" -> "equals" https://codereview.chromium.org/2935503002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:150: annotaionsJsons.put(a.annotationType().getSimpleName(), elementJsonObject); annotaionsJsons https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:369: if test_runner is None and test_runner_junit4 is None: test_runner basically unused here. https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:375: extras['package'] = 'org.chromium' so this won't work downstream right? I think some of the tests have com.google.android package. https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:435: test, sep='#', parameter_postfix='__'): prefix? Or is it more of a parameter_postfix_prefix? jk https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:436: """Gets the name of the given JUnit4 test withouth parameter postfix. withouth https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:447: parameterization_sep: the character(s) that seperate method name and method Update this variable name to what is actually is. parameterization_sep -> parameter_postfix_prefix https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance_test.py (right): https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:650: 'RunWith': {'value': 'class J4Runner'}, intentation looks pretty weird here. https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:653: 'timeout': '0'}, extra space here.
https://codereview.chromium.org/2935503002/diff/80001/base/test/android/javat... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:126: * or "hashCode". On 2017/07/18 at 23:59:11, mikecase wrote: > Smallest nit of all time: "equal()" -> "equals" Done https://codereview.chromium.org/2935503002/diff/80001/base/test/android/javat... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:150: annotaionsJsons.put(a.annotationType().getSimpleName(), elementJsonObject); On 2017/07/18 at 23:59:11, mikecase wrote: > annotaionsJsons Done https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:369: if test_runner is None and test_runner_junit4 is None: On 2017/07/18 at 23:59:12, mikecase wrote: > test_runner basically unused here. Removed https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:375: extras['package'] = 'org.chromium' On 2017/07/18 at 23:59:12, mikecase wrote: > so this won't work downstream right? I think some of the tests have com.google.android package. Changed This is just for the sake of speed up java class lookup https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:435: test, sep='#', parameter_postfix='__'): On 2017/07/18 at 23:59:12, mikecase wrote: > prefix? Or is it more of a parameter_postfix_prefix? jk woah troll https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:436: """Gets the name of the given JUnit4 test withouth parameter postfix. On 2017/07/18 at 23:59:12, mikecase wrote: > withouth Done https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance.py:447: parameterization_sep: the character(s) that seperate method name and method On 2017/07/18 at 23:59:11, mikecase wrote: > Update this variable name to what is actually is. > > parameterization_sep -> parameter_postfix_prefix Done https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... File build/android/pylib/instrumentation/instrumentation_test_instance_test.py (right): https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:650: 'RunWith': {'value': 'class J4Runner'}, On 2017/07/18 at 23:59:12, mikecase wrote: > intentation looks pretty weird here. Done https://codereview.chromium.org/2935503002/diff/80001/build/android/pylib/ins... build/android/pylib/instrumentation/instrumentation_test_instance_test.py:653: 'timeout': '0'}, On 2017/07/18 at 23:59:12, mikecase wrote: > extra space here. Done
The CQ bit was checked by yolandyan@chromium.org 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 yolandyan@chromium.org
The CQ bit was checked by yolandyan@chromium.org 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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Patchset #7 (id:120001) has been deleted
final comments. https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:40: private final Map<Class<?>, List<JSONObject>> mTestClassJSONObjects = new HashMap<>(); why not JSONArray instead of List<JSONObject>? https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:56: mTestClassJSONObjects.get(desc.getTestClass()).add(getTestMethodJSON(desc)); I thought this was kinda confusing the way the code is split up between testStarted and testRunFinished. I don't think there is much purpose for mTestClassJSONObjects. This could just be the base JSONObject. And then you could do like.... if (!mBaseJSONObject.has(testclass)) { // Create the class JSON like you do in } classJSON = mBaseJSONObject.get(testclass) classJSON.get('methods').put(getTestMethodJSON(desc)) or something? https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:64: List<JSONObject> allTestClasses = new LinkedList<>(); Could this just be a JSONArray instead of creating a List<JSONObject> and using it to initialize the JSONArray later? https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:74: allTestClasses.add(classObject); As mentioned, I think the code would be more nicely organized if the above logic lived in testStarted()
The CQ bit was checked by yolandyan@chromium.org 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/2935503002/diff/140001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2935503002/diff/140001/base/BUILD.gn#newcode2698 base/BUILD.gn:2698: "test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java", nit: alphabetize https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:74: allTestClasses.add(classObject); On 2017/07/19 17:11:01, mikecase wrote: > As mentioned, I think the code would be more nicely organized if the above logic > lived in testStarted() w/ this suggestion, testRunFinished would just be responsible for writing the JSON to file? Sounds pretty reasonable to me. https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:260: logging.info('Getting tests by print tests in instrumentation runner') nit: 'Getting tests by having %s list them.' % junit4_runner_class https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:365: def _GetAllTestsFromRunner(device, test_package, junit4_runner_class): Sorry, missed this the first time around. Device-specific behavior does not belong in the test instance. This should be handled in the test_run. https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:367: device.adb, suffix='.json', dir=device.GetExternalStoragePath()) as f: nit: this needs a more descriptive name than "f" given the extent of the scope below. https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:374: target, extras=extras, timeout=30, retries=2)) I mentioned retries here, but I should say that neither timeout nor retries needs to be overridden here.
Patchset #8 (id:160001) has been deleted
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/2935503002/diff/140001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2935503002/diff/140001/base/BUILD.gn#newcode2698 base/BUILD.gn:2698: "test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java", On 2017/07/19 at 20:43:34, jbudorick wrote: > nit: alphabetize Done https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:40: private final Map<Class<?>, List<JSONObject>> mTestClassJSONObjects = new HashMap<>(); On 2017/07/19 at 17:11:01, mikecase wrote: > why not JSONArray instead of List<JSONObject>? I think this was done for perf reasons, but I don't think it matters much Changed https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:56: mTestClassJSONObjects.get(desc.getTestClass()).add(getTestMethodJSON(desc)); On 2017/07/19 at 17:11:01, mikecase wrote: > I thought this was kinda confusing the way the code is split up between testStarted and testRunFinished. > > I don't think there is much purpose for mTestClassJSONObjects. This could just be the base JSONObject. And then you could do like.... > > if (!mBaseJSONObject.has(testclass)) { > // Create the class JSON like you do in > } > classJSON = mBaseJSONObject.get(testclass) > classJSON.get('methods').put(getTestMethodJSON(desc)) > > or something? Done https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:64: List<JSONObject> allTestClasses = new LinkedList<>(); On 2017/07/19 at 17:11:01, mikecase wrote: > Could this just be a JSONArray instead of creating a List<JSONObject> and using it to initialize the JSONArray later? Done https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:74: allTestClasses.add(classObject); On 2017/07/19 at 17:11:01, mikecase wrote: > As mentioned, I think the code would be more nicely organized if the above logic lived in testStarted() Done https://codereview.chromium.org/2935503002/diff/140001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:74: allTestClasses.add(classObject); On 2017/07/19 at 20:43:34, jbudorick wrote: > On 2017/07/19 17:11:01, mikecase wrote: > > As mentioned, I think the code would be more nicely organized if the above logic > > lived in testStarted() > > w/ this suggestion, testRunFinished would just be responsible for writing the JSON to file? > > Sounds pretty reasonable to me. Done https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:260: logging.info('Getting tests by print tests in instrumentation runner') On 2017/07/19 20:43:34, jbudorick wrote: > nit: 'Getting tests by having %s list them.' % junit4_runner_class Done. https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:365: def _GetAllTestsFromRunner(device, test_package, junit4_runner_class): On 2017/07/19 at 20:43:34, jbudorick wrote: > Sorry, missed this the first time around. Device-specific behavior does not belong in the test instance. This should be handled in the test_run. Done https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:367: device.adb, suffix='.json', dir=device.GetExternalStoragePath()) as f: On 2017/07/19 at 20:43:34, jbudorick wrote: > nit: this needs a more descriptive name than "f" given the extent of the scope below. Done https://codereview.chromium.org/2935503002/diff/140001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:374: target, extras=extras, timeout=30, retries=2)) On 2017/07/19 at 20:43:34, jbudorick wrote: > I mentioned retries here, but I should say that neither timeout nor retries needs to be overridden here. Done
The CQ bit was checked by yolandyan@chromium.org 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.
Patchset #10 (id:240001) has been deleted
Patchset #9 (id:220001) has been deleted
lgtm if you remove changes to android_webview/javatests/AndroidManifest.xml
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
On 2017/07/20 at 21:20:28, mikecase wrote: > lgtm if you remove changes to android_webview/javatests/AndroidManifest.xml Done
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yolandyan@chromium.org 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...
Change since last comment: https://codereview.chromium.org/2935503002/diff2/140001:280001/base/BUILD.gn
https://codereview.chromium.org/2935503002/diff/280001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/TestListInstrumentationRunListenerTest.java (right): https://codereview.chromium.org/2935503002/diff/280001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/TestListInstrumentationRunListenerTest.java:107: System.out.println(json.toString()); probably dont want println in test
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2935503002/diff/280001/base/test/android/juni... File base/test/android/junit/src/org/chromium/base/test/TestListInstrumentationRunListenerTest.java (right): https://codereview.chromium.org/2935503002/diff/280001/base/test/android/juni... base/test/android/junit/src/org/chromium/base/test/TestListInstrumentationRunListenerTest.java:107: System.out.println(json.toString()); On 2017/07/21 at 18:47:13, mikecase wrote: > probably dont want println in test Removed
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2935503002/diff/280001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/280001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:78: throw e; nit: Do we need to catch this if we're just going to rethrow it...? Also, if you're going to do so, I think this can just be throw; https://codereview.chromium.org/2935503002/diff/280001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/280001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:44: _TEST_WITHOUT_SIZE_ANNOTATIONS = [ what the... what are these? Where did they come from? https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:258: def _GetAllTestsFromRunnerPickle(test_apk): Why is this a separate function? https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:896: def GetAllTestsFromRunnerPickle(self): I think this should just be GetTestsFromPickle. Let the test_run handle the different name. https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:314: self._SaveTestsFromRunner() This is more complicated than it needs to be. Rather than saving the tests into the instance and then getting them right back on the next line, just get them here. Don't worry about saving them up into the instance in between. https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:561: def _SaveTestsFromRunner(self): This should be _GetTestsFromRunner https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:583: if output_string: What could this output that would indicate failure in a way that wouldn't be caught by our normal error detection mechanisms? https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:584: raise Exception('Test listing through %s failed on device:\n%s' % ( I don't think this should be a raw Exception. Maybe a CommandFailedError, or a subclass thereof? https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:592: self._test_instance.SaveTestsToPickle(raw_tests) nit: do this outside of the temporary context managers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:258: def _GetAllTestsFromRunnerPickle(test_apk): On 2017/07/21 at 19:35:08, jbudorick wrote: > Why is this a separate function? 42 https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:896: def GetAllTestsFromRunnerPickle(self): On 2017/07/21 at 19:35:08, jbudorick wrote: > I think this should just be GetTestsFromPickle. Let the test_run handle the different name. Done https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:314: self._SaveTestsFromRunner() On 2017/07/21 at 19:35:08, jbudorick wrote: > This is more complicated than it needs to be. Rather than saving the tests into the instance and then getting them right back on the next line, just get them here. Don't worry about saving them up into the instance in between. Done https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:561: def _SaveTestsFromRunner(self): On 2017/07/21 at 19:35:08, jbudorick wrote: > This should be _GetTestsFromRunner Done https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:583: if output_string: On 2017/07/21 at 19:35:08, jbudorick wrote: > What could this output that would indicate failure in a way that wouldn't be caught by our normal error detection mechanisms? I had incidents where it printed out the tests because I didn't set up JUnit runner right. https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:584: raise Exception('Test listing through %s failed on device:\n%s' % ( On 2017/07/21 at 19:35:08, jbudorick wrote: > I don't think this should be a raw Exception. Maybe a CommandFailedError, or a subclass thereof? Done https://codereview.chromium.org/2935503002/diff/300001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:592: self._test_instance.SaveTestsToPickle(raw_tests) On 2017/07/21 at 19:35:08, jbudorick wrote: > nit: do this outside of the temporary context managers. Done
The CQ bit was checked by yolandyan@chromium.org 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.
The CQ bit was checked by yolandyan@chromium.org 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/2935503002/diff/340001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:812: def GetTests(self, raw_tests=None): This is a bit odd -- GetTests where the caller provides the tests? Maybe either: - rename this function; or - split test inflation etc into a separate function called by both GetTests here and the test run's GetTests? https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:574: device = self._env.devices[0] We switched gtests over to using all devices a while back: https://codesearch.chromium.org/chromium/src/build/android/pylib/local/device... https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:586: if output_string: Do we expect this to happen as part of normal operation, or was it just a debug issue that you've since resolved? https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:594: json_string = host_file.read() nit: raw_tests = json.load(host_file)
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:574: device = self._env.devices[0] On 2017/07/24 at 04:01:06, jbudorick wrote: > We switched gtests over to using all devices a while back: https://codesearch.chromium.org/chromium/src/build/android/pylib/local/device... Cool, didn't know that https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:586: if output_string: On 2017/07/24 at 04:01:06, jbudorick wrote: > Do we expect this to happen as part of normal operation, or was it just a debug issue that you've since resolved? It should never happen with BaseChromiumAndroidJUnitRunner, but if someone accidentally use AndroidJUnitRunner in manifest. This would cause problem. Changed the error message accordingly ("Test listing through %s failed on device:\n%s. Are you using the right JUnit runner? (org.chromium.base.test.BaseChromiumAndroidJUnitRunner)"). https://codereview.chromium.org/2935503002/diff/340001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:594: json_string = host_file.read() On 2017/07/24 at 04:01:06, jbudorick wrote: > nit: raw_tests = json.load(host_file) Done
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:608: raw_tests = [tl for tl in raw_test_lists if tl][0] Here I am simply getting the first viable raw test list, since it's hard to compare and get union of multiple list of dictionaries, and they will all be the same anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nits https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:833: super nit: -1 blank line https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:576: def list_tests(dev): nit: this is a lot to do in an exception; could you instead tweak it to be: try: return GetTestsFromPickle... except ...: logging.info('Could not get tests from pickle') logging.info('Getting tests by...') def list_tests(dev): ... ... return raw_tests ? https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:590: ('Test listing through %s failed on dev:\n%s. Are you using' nit: for a multi-line string like this with multiple replacements, use .format rather than % and consider building it prior to raising the exception
https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/in... File build/android/pylib/instrumentation/instrumentation_test_instance.py (right): https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/in... build/android/pylib/instrumentation/instrumentation_test_instance.py:833: On 2017/07/24 at 22:24:38, jbudorick wrote: > super nit: -1 blank line Done https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... File build/android/pylib/local/device/local_device_instrumentation_test_run.py (right): https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:576: def list_tests(dev): On 2017/07/24 at 22:24:38, jbudorick wrote: > nit: this is a lot to do in an exception; could you instead tweak it to be: > > try: > return GetTestsFromPickle... > except ...: > logging.info('Could not get tests from pickle') > > logging.info('Getting tests by...') > def list_tests(dev): > ... > > ... > return raw_tests > > ? Done https://codereview.chromium.org/2935503002/diff/360001/build/android/pylib/lo... build/android/pylib/local/device/local_device_instrumentation_test_run.py:590: ('Test listing through %s failed on dev:\n%s. Are you using' On 2017/07/24 at 22:24:38, jbudorick wrote: > nit: for a multi-line string like this with multiple replacements, use .format rather than % and consider building it prior to raising the exception Done
The CQ bit was checked by yolandyan@chromium.org 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.
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2935503002/#ps380001 (title: "address 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2935503002/#ps400001 (title: "Rebase")
The CQ bit was unchecked by yolandyan@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yolandyan@chromium.org 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yolandyan@chromium.org to run a CQ dry run
The CQ bit was unchecked by yolandyan@chromium.org
The CQ bit was checked by yolandyan@chromium.org 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.
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2935503002/#ps420001 (title: "fix error")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yolandyan@chromium.org changed reviewers: + nyquist@chromium.org
add +nyquist for OWNER stamp on base/ files
Description was changed from ========== List Java Instru Test Information From JUnit Runner This CL allow test information that was once listed by parsing proguard dump or dexdump to be produced by JUnit runner run all the tests without executing them. The TestListInstrumentationRunListener is registered in the BaseChromiumAndroidJUnitRunner and will write all tests to a json file which then will be pulled to host side by host side runner script BUG=640116 ========== to ========== List Java Instru Test Information From JUnit Runner This CL allow test information that was once listed by parsing proguard dump or dexdump to be produced by JUnit runner run all the tests without executing them. The TestListInstrumentationRunListener is registered in the BaseChromiumAndroidJUnitRunner and will write all tests to a json file which then will be pulled to host side by host side runner script BUG=640116 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by yolandyan@chromium.org 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...
yolandyan@chromium.org changed reviewers: + xunjieli@chromium.org
add +xunjieli for cronet OWNER stamp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm % comments https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java (right): https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:29: private Bundle mArguments; non-static member should go after static members https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:59: listTests(); Instead of doing the else-statement here, do you think you could instead add a return statement here to signify that we intentionally do an early return before super.onStart(), and maybe even add a short comment as to why we never want to run onStart() here? https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:74: final String msg = "Fatal exception when running tests"; Nit: Unnecessary final https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:51: if (!mTestClassJsonMap.containsKey(desc.getTestClass())) { Optional nit: Why is this if-check inverted? It's much easier to read without the ! at the start. https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:76: } catch (Exception e) { Do the statements above really 'throw new Exception(...)'? If not, could you please catch the particular exceptions instead of this catchall that also includes RuntimeException? and if you want RuntimeException as well, just include that by specifying it manually. https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:83: } catch (IOException e) { Add a comment to say that were are intentionally ignoring the IOException
The CQ bit was checked by yolandyan@chromium.org 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...
https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java (right): https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:29: private Bundle mArguments; On 2017/07/26 at 17:25:07, nyquist wrote: > non-static member should go after static members Done https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:59: listTests(); On 2017/07/26 at 17:25:07, nyquist wrote: > Instead of doing the else-statement here, do you think you could instead add a return statement here to signify that we intentionally do an early return before super.onStart(), and maybe even add a short comment as to why we never want to run onStart() here? Done https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/BaseChromiumAndroidJUnitRunner.java:74: final String msg = "Fatal exception when running tests"; On 2017/07/26 at 17:25:07, nyquist wrote: > Nit: Unnecessary final Done https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... File base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java (right): https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:51: if (!mTestClassJsonMap.containsKey(desc.getTestClass())) { On 2017/07/26 at 17:25:07, nyquist wrote: > Optional nit: Why is this if-check inverted? It's much easier to read without the ! at the start. Done https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:76: } catch (Exception e) { On 2017/07/26 at 17:25:07, nyquist wrote: > Do the statements above really 'throw new Exception(...)'? If not, could you please catch the particular exceptions instead of this catchall that also includes RuntimeException? and if you want RuntimeException as well, just include that by specifying it manually. Changed to IOException https://codereview.chromium.org/2935503002/diff/440001/base/test/android/java... base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java:83: } catch (IOException e) { On 2017/07/26 at 17:25:07, nyquist wrote: > Add a comment to say that were are intentionally ignoring the IOException Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet_tester on master.tryserver.chromium.android (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org, nyquist@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2935503002/#ps460001 (title: "address nyquist comments")
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": 460001, "attempt_start_ts": 1501115163212930,
"parent_rev": "7847afc23c816960a019476e995c696143365cac", "commit_rev":
"682e23394ce75a8b9d19a5ee351f401706e5cff7"}
Message was sent while issue was closed.
Description was changed from ========== List Java Instru Test Information From JUnit Runner This CL allow test information that was once listed by parsing proguard dump or dexdump to be produced by JUnit runner run all the tests without executing them. The TestListInstrumentationRunListener is registered in the BaseChromiumAndroidJUnitRunner and will write all tests to a json file which then will be pulled to host side by host side runner script BUG=640116 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== List Java Instru Test Information From JUnit Runner This CL allow test information that was once listed by parsing proguard dump or dexdump to be produced by JUnit runner run all the tests without executing them. The TestListInstrumentationRunListener is registered in the BaseChromiumAndroidJUnitRunner and will write all tests to a json file which then will be pulled to host side by host side runner script BUG=640116 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2935503002 Cr-Commit-Position: refs/heads/master@{#489826} Committed: https://chromium.googlesource.com/chromium/src/+/682e23394ce75a8b9d19a5ee351f... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:460001) as https://chromium.googlesource.com/chromium/src/+/682e23394ce75a8b9d19a5ee351f...
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:460001) has been created in https://codereview.chromium.org/2991833003/ by awdf@chromium.org. The reason for reverting is: Reverting as the likely cause of many webview layout test runner failures - see crbug.com/749468. |
