New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 875909 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Oct 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression
Proj-VR
Proj-XR



Sign in to add a comment

XR tests not being properly listed

Project Member Reported by bsheedy@chromium.org, Aug 20

Issue description

It looks like the the VR and AR instrumentation tests have started being improperly discovered by the test runner or something. The AR tests are failing due to no tests being listed https://chromium-swarm.appspot.com/task?id=3f70bb59673b0710&refresh=10&show_raw=1 while the VR tests are running, but far fewer tests https://chromium-swarm.appspot.com/task?id=3f63bc0358b61f10&refresh=10&show_raw=1 (it lists 106, normally we're closer to 200).

This started with https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Nougat%20Phone%20Tester/7174 for VR and https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Oreo%20Phone%20Tester/487 for AR.
 
Cc: mar...@mwiacek.com
Bisect points to https://chromium-review.googlesource.com/1154527 as the culprit. Marcin, any idea why this would be the case? It's not like the tests are being skipped - they're simply not being found by the test runner.
Cc: tedc...@chromium.org asimjour@chromium.org agrieve@chromium.org
In the past I had few times situation, that correct changes made problems with internal Google tools and tools required modifications.

This is normally situation which needs investigation from Googlers, I don't have access to https://chromium-swarm.appspot.com.

With this patch I could maybe start from looking with HeadTrackingMode change - maybe replacement is somehow in conflict.

Please help me with this - patch is part of bigger part and it would be big shame to drop it.

can I ask for it or not?
I believe non-Googlers can apply for access to swarming, similar to getting committer access. However, I'm not sure of the exact process of that.

In the meantime, I'm happy to help out by running stuff on swarming for you and reporting back. Alternatively, if you happen to have a rooted Pixel device, you can run the affected tests locally, as the issue reproduced both on swarming and locally.
Cc: yolandyan@chromium.org
After looking more closely at the which tests are missing, I've found the following:

1. Only tests that use test parameterization (to run the same test multiple times in different activity types) are affected. These can be identified by having a __*Activity suffix in test results.
2. Only tests that actually run in multiple activities are affected. For example, if a test class is set up to use parameterization, but a particular test case is only set up to run in ChromeTabbedActivity, the test is properly found.

+yolandyan who implemented the backend stuff for parameterization. Yoland, any idea why switching how enums are handled seems to mess with parameterization?
Cc: -yolandyan@chromium.org
Looks like Yoland actually stopped working on Chromium a while ago. I'll see if I can find another suitable person to ask, otherwise I'll see about digging into this myself.
I've found the root cause of this issue. As part of the process of listing all tests, we end up invoking any annotations and trying to store their return value. If the return type is an array, it attempts to cast it to an Object array, as seen at https://cs.chromium.org/chromium/src/base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java?q=testlistinstrumentationrunlistener&sq=package:chromium&dr=CSs&l=132.

The issue is that int arrays cannot be cast to Object arrays, which causes the cast to throw an error and prevent the test from being added to the list.

I assume we can't use Integer instead of int for enums, so we'll have to special case primitive arrays in the test listing code and convert them to arrays of their corresponding objects.
Marcin - you should be able to resubmit the reverted patch without change once https://chromium-review.googlesource.com/c/chromium/src/+/1185461 makes it in.
Owner: bsheedy@chromium.org
Status: Assigned (was: Untriaged)
Brian, assigning to you for now. Please feel free to assign it to marcin@ after your patch lands.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/57c6bb53233de8aa4cfddefcdda8751f8995e8bb

commit 57c6bb53233de8aa4cfddefcdda8751f8995e8bb
Author: bsheedy <bsheedy@chromium.org>
Date: Fri Aug 24 18:08:30 2018

Fix primitive arrays breaking test listing

Fixes an issue with instrumentation tests that contained annotations
that returned arrays of primitives not being listed. This was being
caused by an unconditional cast to an Object array, which is not allowed
with arrays of primitives. The fix is to detect when we receive an array
of primitives and manually copy it over to an array of primitive
wrappers.

This seems to only have been hit for ints with the recent switch to
using IntDef for enums, but the fix works for any primitive type.

Bug:  875909 
Change-Id: I465b1b7bd8c8ec67229e18be13e6da6295de3baa
Reviewed-on: https://chromium-review.googlesource.com/1185461
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585905}
[modify] https://crrev.com/57c6bb53233de8aa4cfddefcdda8751f8995e8bb/base/test/android/javatests/src/org/chromium/base/test/TestListInstrumentationRunListener.java

Owner: mar...@mwiacek.com
Re-assigning to Marcin now that the fix has been landed.
Labels: -VR-Caught-By-Test XR-Caught-By-Test
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5232b51cedefb030357b9e479625e6cd7c65104c

commit 5232b51cedefb030357b9e479625e6cd7c65104c
Author: Marcin Wiacek <marcin@mwiacek.com>
Date: Tue Oct 02 17:45:17 2018

Reland "Replace enum with @IntDef (javatests)"

This is a reland of 4b3c70824825390f583dec33796a107e024498e4
(https://chromium-review.googlesource.com/c/chromium/src/+/1154527)
after merging
https://chromium-review.googlesource.com/c/chromium/src/+/1185461
fixing
https://bugs.chromium.org/p/chromium/issues/detail?id=875909

Original change's description:
> Replace enum with @IntDef (javatests)
>
> @IntDef/@StringDef annotation are preferred way for declaring
> set of String/int values.
>
> 1. they need less space in APK than enum, see
> https://developer.android.com/topic/performance/reduce-apk-size#remove-enums
> 2. they give more control over allowed values than "static final" values
>
> Main goal of patch is writing "static final" values, enum
> and some classes in one common @IntDef/@StringDef form:
>
> 1. with @IntDef/@StringDef first, @Retention second
>    and related @interface third
> 2. with values inside @interface
> 3. with NUM_ENTRIES declaring number of entries if necessary
> 4. with comment about numbering from 0 without gaps when necessary
> 5. with @Retention(RetentionPolicy.SOURCE)
> 6. without "static final" in the @interface
>
> Change-Id: I6aeb3571f6e72dd3599a4a6055189a227312d5c4
> Reviewed-on: https://chromium-review.googlesource.com/1154527
> Reviewed-by: agrieve <agrieve@chromium.org>
> Reviewed-by: Ted Choc <tedchoc@chromium.org>
> Reviewed-by: Amirhossein Simjour <asimjour@chromium.org>
> Commit-Queue: Marcin WiÄ…cek <marcin@mwiacek.com>
> Cr-Commit-Position: refs/heads/master@{#584198}

BUG= 875909 

TBR=agrieve@chromium.org,tedchoc@chromium.org,asimjour@chromium.org

Change-Id: I11a6acd33b6363ddf792eadfeaac01e0d8513a01
Reviewed-on: https://chromium-review.googlesource.com/c/1189642
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595897}
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadForegroundServiceTest.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadManagerServiceTest.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaBaseTest.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/SuggestionsSheetVisibilityChangeObserverTest.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/EmulatedVrController.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserNavigationTest.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/ChromeTabbedActivityVrTestRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/ChromeTabbedActivityXrTestRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/CustomTabActivityVrTestRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/CustomTabActivityXrTestRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/HeadTrackingMode.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/VrActivityRestrictionRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/WebappActivityVrTestRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/WebappActivityXrTestRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/XrActivityRestriction.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/XrActivityRestrictionRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/rules/XrTestRule.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/util/HeadTrackingUtils.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/util/VrInfoBarUtils.java
[modify] https://crrev.com/5232b51cedefb030357b9e479625e6cd7c65104c/chrome/android/javatests/src/org/chromium/chrome/browser/vr/util/XrTestRuleUtils.java

Status: Fixed (was: Assigned)
Status: Verified (was: Fixed)

Sign in to add a comment