Ban non-functional junit/robolectric imports for native java unittests

Adds jni_generator checks to ensure that junit or robolectric features
that don't work in native java unittests aren't mistakenly used.

Bug: 1056388
Change-Id: Ifb9ce291d0c299c201506efa07990ac61ecd65b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2083623
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749800}

base/android/jni_generator/jni_generator.py

@@ -705,6 +705,14 @@ RE_DISABLED_FEATURES = re.compile(
 
 RE_BANNED_FEATURES = re.compile(r'^Features\.')
 
+RE_BANNED_IMPORTS = re.compile(r'(import (?:'
+                               r'org\.junit\.Rule|'
+                               r'org\.junit\.Test|'
+                               r'org\.junit\.runner|'
+                               r'org\.junit\.Before|'
+                               r'org\.junit\.After|'
+                               r'org\.robolectric))')
+
 RE_FEATURE_STRING = r'[^"]*"(.*)";'
 
 # Removes empty lines that are indented (i.e. start with 2x spaces).
@@ -748,7 +756,8 @@ def _GetFeaturesString(features_match, feature_list_file):
 def ExtractCalledByNatives(jni_params,
                            contents,
                            always_mangle,
-                           feature_list_file=None):
+                           feature_list_file=None,
+                           fully_qualified_class=""):
   """Parses all methods annotated with @CalledByNative.
 
   Args:
@@ -766,6 +775,7 @@ def ExtractCalledByNatives(jni_params,
     ParseError: if unable to parse.
   """
   called_by_natives = []
+  checked_banned_imports = False
   for match in re.finditer(RE_CALLED_BY_NATIVE, contents):
     cbn = None
     enabled_features = None
@@ -780,10 +790,20 @@ def ExtractCalledByNatives(jni_params,
       if re.match(RE_BANNED_FEATURES, value):
         raise ParseError(
             '@Features.EnableFeatures will not work, please use '
-            '@NativeJavaTestFeatures.Enable("MyFeature") instead.', value)
+            '@NativeJavaTestFeatures.Enable("MyFeature") instead.',
+            fully_qualified_class, value)
     if not cbn:
       continue
     java_test = 'JavaTest' in cbn.group('JavaTest')
+    if java_test and not checked_banned_imports:
+      checked_banned_imports = True
+      imports = re.search(RE_BANNED_IMPORTS, contents)
+      if imports:
+        raise ParseError(
+            'Most junit and robolectric features do not work for native java '
+            'unittests (asserts are okay).', fully_qualified_class,
+            imports.group(1))
+
 
     enabled_string = _GetFeaturesString(enabled_features, feature_list_file)
     disabled_string = _GetFeaturesString(disabled_features, feature_list_file)
@@ -791,7 +811,7 @@ def ExtractCalledByNatives(jni_params,
     if not java_test and (enabled_string or disabled_string):
       raise ParseError(
           '@NativeJavaTestFeatures requires @CalledByNativeJavaTest to work.',
-          match.group('annotations'))
+          fully_qualified_class, match.group('annotations'))
 
     return_type = match.group('return_type')
     name = match.group('name')
@@ -822,7 +842,7 @@ def ExtractCalledByNatives(jni_params,
   for line1, line2 in zip(unmatched_lines, unmatched_lines[1:]):
     if '@CalledByNative' in line1:
       raise ParseError('could not parse @CalledByNative method signature',
-                       line1, line2)
+                       fully_qualified_class, line1, line2)
   return MangleCalledByNatives(jni_params, called_by_natives, always_mangle)
 
 
@@ -1039,9 +1059,9 @@ class JNIFromJavaSource(object):
     self.jni_params.ExtractImportsAndInnerClasses(contents)
     jni_namespace = ExtractJNINamespace(contents) or options.namespace
     natives = ExtractNatives(contents, options.ptr_type)
-    called_by_natives = ExtractCalledByNatives(self.jni_params, contents,
-                                               options.always_mangle,
-                                               options.feature_list_file)
+    called_by_natives = ExtractCalledByNatives(
+        self.jni_params, contents, options.always_mangle,
+        options.feature_list_file, fully_qualified_class)
 
     natives += ProxyHelpers.ExtractStaticProxyNatives(
         fully_qualified_class, contents, options.ptr_type,

base/android/jni_generator/jni_generator_tests.py

@@ -818,7 +818,30 @@ scooby doo
           always_mangle=False)
       self.fail('Expected a ParseError')
     except jni_generator.ParseError as e:
-      self.assertEqual(('@CalledByNative', 'scooby doo'), e.context_lines)
+      self.assertEqual(('', '@CalledByNative', 'scooby doo'), e.context_lines)
+
+  def testCalledByNativeJavaTestImportErrors(self):
+    # Using banned imports
+    try:
+      jni_params = jni_generator.JniParams('')
+      jni_generator.ExtractCalledByNatives(
+          jni_params,
+          """
+import org.junit.Rule;
+
+class MyClass {
+    @Rule
+    public JniMocker mocker = new JniMocker();
+
+    @CalledByNativeJavaTest
+    public void testStuff() {}
+}
+""",
+          always_mangle=False,
+          feature_list_file=_FeatureListFile())
+      self.fail('Expected a ParseError')
+    except jni_generator.ParseError as e:
+      self.assertEqual(('', 'import org.junit.Rule'), e.context_lines)
 
   def testCalledByNativeJavaTestFeatureParseErrors(self):
     # Using banned Features.Enable/Disable
@@ -838,7 +861,7 @@ class MyClass {
       self.fail('Expected a ParseError')
     except jni_generator.ParseError as e:
       self.assertEqual(
-          ('Features.Disable({ChromeFeatureList.SOME_FEATURE})\n    ', ),
+          ('', 'Features.Disable({ChromeFeatureList.SOME_FEATURE})\n    '),
           e.context_lines)
 
     # Using NativeJavaTestFeatures outside of a test.
@@ -856,8 +879,11 @@ public void testNotActuallyATest() {}
           feature_list_file=_FeatureListFile())
       self.fail('Expected a ParseError')
     except jni_generator.ParseError as e:
-      self.assertEqual(('@CalledByNative @NativeJavaTestFeatures.Enable('
-                        'TestFeatureList.MY_FEATURE) ', ), e.context_lines)
+      self.assertEqual((
+          '',
+          '@CalledByNative @NativeJavaTestFeatures.Enable('
+          'TestFeatureList.MY_FEATURE) ',
+      ), e.context_lines)
 
     # Not specifying a feature_list_file.
     try:

chrome/android/native_java_unittests/src/org/chromium/chrome/browser/installedapp/InstalledAppProviderTest.java

@@ -13,17 +13,13 @@ import android.os.Bundle;
 import android.util.Pair;
 
 import org.junit.Assert;
-import org.junit.Rule;
 
 import org.chromium.base.Callback;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.CalledByNativeJavaTest;
-import org.chromium.base.annotations.DisabledCalledByNativeJavaTest;
-import org.chromium.base.test.util.JniMocker;
 import org.chromium.chrome.browser.UnitTestUtils;
 import org.chromium.chrome.browser.instantapps.InstantAppsHandler;
 import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.installedapp.mojom.InstalledAppProvider;
 import org.chromium.installedapp.mojom.RelatedApplication;
 import org.chromium.url.URI;
@@ -76,9 +72,7 @@ public class InstalledAppProviderTest {
     private FakeFrameUrlDelegate mFrameUrlDelegate;
     private InstalledAppProviderTestImpl mInstalledAppProvider;
     private FakeInstantAppsHandler mFakeInstantAppsHandler;
-
-    @Rule
-    public JniMocker mocker = new JniMocker();
+    private TestInstalledAppProviderImplJni mTestInstalledAppProviderImplJni;
 
     private static class FakePackageManager extends PackageManagerDelegate {
         private Map<String, PackageInfo> mPackageInfo = new HashMap<>();
@@ -140,7 +134,7 @@ public class InstalledAppProviderTest {
         @Override
         protected void delayThenRun(Runnable r, long delayMillis) {
             mLastDelayMillis = delayMillis;
-            r.run();
+            super.delayThenRun(r, 0);
         }
 
         @Override
@@ -156,17 +150,17 @@ public class InstalledAppProviderTest {
 
     private static class TestInstalledAppProviderImplJni
             implements InstalledAppProviderImpl.Natives {
-        private static final Map<String, String> RELATION_MAP = new HashMap<>();
+        private final Map<String, String> mRelationMap = new HashMap<>();
 
-        public static void addVerfication(String webDomain, String manifestUrl) {
-            RELATION_MAP.put(webDomain, manifestUrl);
+        public void addVerfication(String webDomain, String manifestUrl) {
+            mRelationMap.put(webDomain, manifestUrl);
         }
 
         @Override
         public void checkDigitalAssetLinksRelationshipForWebApk(
                 Profile profile, String webDomain, String manifestUrl, Callback<Boolean> callback) {
-            callback.onResult(RELATION_MAP.containsKey(webDomain)
-                    && RELATION_MAP.get(webDomain).equals(manifestUrl));
+            callback.onResult(mRelationMap.containsKey(webDomain)
+                    && mRelationMap.get(webDomain).equals(manifestUrl));
         }
     }
 
@@ -386,9 +380,9 @@ public class InstalledAppProviderTest {
 
     @CalledByNative
     public void setUp() throws Exception {
-        TestThreadUtils.setThreadAssertsDisabled(true);
-
-        mocker.mock(InstalledAppProviderImplJni.TEST_HOOKS, new TestInstalledAppProviderImplJni());
+        mTestInstalledAppProviderImplJni = new TestInstalledAppProviderImplJni();
+        InstalledAppProviderImplJni.TEST_HOOKS.setInstanceForTesting(
+                mTestInstalledAppProviderImplJni);
 
         mFakePackageManager = new FakePackageManager();
         mFrameUrlDelegate = new FakeFrameUrlDelegate(URL_ON_ORIGIN);
@@ -397,6 +391,11 @@ public class InstalledAppProviderTest {
                 mFrameUrlDelegate, mFakePackageManager, mFakeInstantAppsHandler);
     }
 
+    @CalledByNative
+    public void tearDown() throws Exception {
+        InstalledAppProviderImplJni.TEST_HOOKS.setInstanceForTesting(null);
+    }
+
     /** Origin of the page using the API is missing certain parts of the URI. */
     @CalledByNativeJavaTest
     public void testOriginMissingParts() throws Exception {
@@ -865,7 +864,7 @@ public class InstalledAppProviderTest {
     }
 
     /** Tests the pseudo-random artificial delay to counter a timing attack. */
-    @DisabledCalledByNativeJavaTest  // crbug.com/1058857
+    @CalledByNativeJavaTest
     public void testArtificialDelay() throws Exception {
         byte[] salt = {0x64, 0x09, -0x68, -0x25, 0x70, 0x11, 0x25, 0x24, 0x68, -0x1a, 0x08, 0x79,
                 -0x12, -0x50, 0x3b, -0x57, -0x17, -0x4d, 0x46, 0x02};
@@ -974,7 +973,7 @@ public class InstalledAppProviderTest {
 
         verifyInstalledApps(manifestRelatedApps, new RelatedApplication[] {});
 
-        TestInstalledAppProviderImplJni.addVerfication(OTHER_MANIFEST_URL, MANIFEST_URL);
+        mTestInstalledAppProviderImplJni.addVerfication(OTHER_MANIFEST_URL, MANIFEST_URL);
         verifyInstalledApps(manifestRelatedApps, new RelatedApplication[] {webApk});
     }
 
@@ -987,7 +986,7 @@ public class InstalledAppProviderTest {
                 createRelatedApplication(PLATFORM_WEBAPP, null, OTHER_MANIFEST_URL);
         RelatedApplication manifestRelatedApps[] = new RelatedApplication[] {webApk};
 
-        TestInstalledAppProviderImplJni.addVerfication(MANIFEST_URL, OTHER_MANIFEST_URL);
+        mTestInstalledAppProviderImplJni.addVerfication(MANIFEST_URL, OTHER_MANIFEST_URL);
         verifyInstalledApps(manifestRelatedApps, new RelatedApplication[] {});
     }
 }

chrome/browser/installedapp/installed_app_provider_unittest.cc

@@ -18,6 +18,10 @@ class InstalledAppProviderTest : public ::testing::Test {
     Java_InstalledAppProviderTest_setUp(AttachCurrentThread(), j_test_);
   }
 
+  void TearDown() override {
+    Java_InstalledAppProviderTest_tearDown(AttachCurrentThread(), j_test_);
+  }
+
   const base::android::ScopedJavaGlobalRef<jobject>& j_test() {
     return j_test_;
   }