AAPT2: Forward @TestApi in resource comments to JavaDoc

Bug: 37894597
Test: make aapt2_tests
Change-Id: I357fb84941bfbb3892a8c46feb47f55b865b6649

7 files changed, 186 insertions, 190 deletions

diff --git a/tools/aapt2/java/AnnotationProcessor.cpp b/tools/aapt2/java/AnnotationProcessor.cpp
index 1f83fa098d74..c93461a66899 100644
--- a/tools/aapt2/java/AnnotationProcessor.cpp
+++ b/tools/aapt2/java/AnnotationProcessor.cpp
@@ -17,6 +17,7 @@
 #include "java/AnnotationProcessor.h"
 
 #include <algorithm>
+#include <array>
 
 #include "text/Unicode.h"
 #include "text/Utf8Iterator.h"
@@ -41,30 +42,54 @@ StringPiece AnnotationProcessor::ExtractFirstSentence(const StringPiece& comment
   return comment;
 }
 
-void AnnotationProcessor::AppendCommentLine(std::string& comment) {
+struct AnnotationRule {
+  enum : uint32_t {
+    kDeprecated = 0x01,
+    kSystemApi = 0x02,
+    kTestApi = 0x04,
+  };
+
+  StringPiece doc_str;
+  uint32_t bit_mask;
+  StringPiece annotation;
+};
+
+static std::array<AnnotationRule, 2> sAnnotationRules = {{
+    {"@SystemApi", AnnotationRule::kSystemApi, "@android.annotation.SystemApi"},
+    {"@TestApi", AnnotationRule::kTestApi, "@android.annotation.TestApi"},
+}};
+
+void AnnotationProcessor::AppendCommentLine(std::string comment) {
   static const std::string sDeprecated = "@deprecated";
-  static const std::string sSystemApi = "@SystemApi";
 
+  // Treat deprecated specially, since we don't remove it from the source comment.
   if (comment.find(sDeprecated) != std::string::npos) {
-    annotation_bit_mask_ |= kDeprecated;
+    annotation_bit_mask_ |= AnnotationRule::kDeprecated;
   }
 
-  std::string::size_type idx = comment.find(sSystemApi);
-  if (idx != std::string::npos) {
-    annotation_bit_mask_ |= kSystemApi;
-    comment.erase(comment.begin() + idx,
-                  comment.begin() + idx + sSystemApi.size());
+  for (const AnnotationRule& rule : sAnnotationRules) {
+    std::string::size_type idx = comment.find(rule.doc_str.data());
+    if (idx != std::string::npos) {
+      annotation_bit_mask_ |= rule.bit_mask;
+      comment.erase(comment.begin() + idx, comment.begin() + idx + rule.doc_str.size());
+    }
   }
 
-  if (util::TrimWhitespace(comment).empty()) {
+  // Check if after removal of annotations the line is empty.
+  const StringPiece trimmed = util::TrimWhitespace(comment);
+  if (trimmed.empty()) {
     return;
   }
 
+  // If there was trimming to do, copy the string.
+  if (trimmed.size() != comment.size()) {
+    comment = trimmed.to_string();
+  }
+
   if (!has_comments_) {
     has_comments_ = true;
     comment_ << "/**";
   }
-
   comment_ << "\n * " << std::move(comment);
 }
 
@@ -73,16 +98,18 @@ void AnnotationProcessor::AppendComment(const StringPiece& comment) {
   for (StringPiece line : util::Tokenize(comment, '\n')) {
     line = util::TrimWhitespace(line);
     if (!line.empty()) {
-      std::string lineCopy = line.to_string();
-      AppendCommentLine(lineCopy);
+      AppendCommentLine(line.to_string());
     }
   }
 }
 
-void AnnotationProcessor::AppendNewLine() { comment_ << "\n *"; }
+void AnnotationProcessor::AppendNewLine() {
+  if (has_comments_) {
+    comment_ << "\n *";
+  }
+}
 
-void AnnotationProcessor::WriteToStream(std::ostream* out,
-                                        const StringPiece& prefix) const {
+void AnnotationProcessor::WriteToStream(const StringPiece& prefix, std::ostream* out) const {
   if (has_comments_) {
     std::string result = comment_.str();
     for (StringPiece line : util::Tokenize(result, '\n')) {
@@ -92,12 +119,14 @@ void AnnotationProcessor::WriteToStream(std::ostream* out,
          << "\n";
   }
 
-  if (annotation_bit_mask_ & kDeprecated) {
+  if (annotation_bit_mask_ & AnnotationRule::kDeprecated) {
     *out << prefix << "@Deprecated\n";
   }
 
-  if (annotation_bit_mask_ & kSystemApi) {
-    *out << prefix << "@android.annotation.SystemApi\n";
+  for (const AnnotationRule& rule : sAnnotationRules) {
+    if (annotation_bit_mask_ & rule.bit_mask) {
+      *out << prefix << rule.annotation << "\n";
+    }
   }
 }
 
diff --git a/tools/aapt2/java/AnnotationProcessor.h b/tools/aapt2/java/AnnotationProcessor.h
index a06eda0f9c5c..a7bf73f50de5 100644
--- a/tools/aapt2/java/AnnotationProcessor.h
+++ b/tools/aapt2/java/AnnotationProcessor.h
@@ -24,64 +24,53 @@
 
 namespace aapt {
 
-/**
- * Builds a JavaDoc comment from a set of XML comments.
- * This will also look for instances of @SystemApi and convert them to
- * actual Java annotations.
- *
- * Example:
- *
- * Input XML:
- *
- * <!-- This is meant to be hidden because
- *      It is system api. Also it is @deprecated
- *      @SystemApi
- *      -->
- *
- * Output JavaDoc:
- *
- *  /\*
- *   * This is meant to be hidden because
- *   * It is system api. Also it is @deprecated
- *   *\/
- *
- * Output Annotations:
- *
- * @Deprecated
- * @android.annotation.SystemApi
- *
- */
+// Builds a JavaDoc comment from a set of XML comments.
+// This will also look for instances of @SystemApi and convert them to
+// actual Java annotations.
+//
+// Example:
+//
+// Input XML:
+//
+// <!-- This is meant to be hidden because
+//      It is system api. Also it is @deprecated
+//      @SystemApi
+//      -->
+//
+// Output JavaDoc:
+//
+//  /**
+//   * This is meant to be hidden because
+//   * It is system api. Also it is @deprecated
+//   */
+//
+// Output Annotations:
+//
+// @Deprecated
+// @android.annotation.SystemApi
 class AnnotationProcessor {
  public:
+  // Extracts the first sentence of a comment. The algorithm selects the substring starting from
+  // the beginning of the string, and ending at the first '.' character that is followed by a
+  // whitespace character. If these requirements are not met, the whole string is returned.
   static android::StringPiece ExtractFirstSentence(const android::StringPiece& comment);
 
-  /**
-   * Adds more comments. Since resources can have various values with different
-   * configurations,
-   * we need to collect all the comments.
-   */
+  // Adds more comments. Resources can have value definitions for various configurations, and
+  // each of the definitions may have comments that need to be processed.
   void AppendComment(const android::StringPiece& comment);
 
   void AppendNewLine();
 
-  /**
-   * Writes the comments and annotations to the stream, with the given prefix
-   * before each line.
-   */
-  void WriteToStream(std::ostream* out, const android::StringPiece& prefix) const;
+  // Writes the comments and annotations to the stream, with the given prefix before each line.
+  void WriteToStream(const android::StringPiece& prefix, std::ostream* out) const;
 
  private:
-  enum : uint32_t {
-    kDeprecated = 0x01,
-    kSystemApi = 0x02,
-  };
-
   std::stringstream comment_;
   std::stringstream mAnnotations;
   bool has_comments_ = false;
   uint32_t annotation_bit_mask_ = 0;
 
-  void AppendCommentLine(std::string& line);
+  void AppendCommentLine(std::string line);
 };
 
 }  // namespace aapt
diff --git a/tools/aapt2/java/AnnotationProcessor_test.cpp b/tools/aapt2/java/AnnotationProcessor_test.cpp
index 9ccac8888426..856f4ccbd7f0 100644
--- a/tools/aapt2/java/AnnotationProcessor_test.cpp
+++ b/tools/aapt2/java/AnnotationProcessor_test.cpp
@@ -34,7 +34,7 @@ TEST(AnnotationProcessorTest, EmitsDeprecated) {
   processor.AppendComment(comment);
 
   std::stringstream result;
-  processor.WriteToStream(&result, "");
+  processor.WriteToStream("", &result);
   std::string annotations = result.str();
 
   EXPECT_THAT(annotations, HasSubstr("@Deprecated"));
@@ -45,7 +45,7 @@ TEST(AnnotationProcessorTest, EmitsSystemApiAnnotationAndRemovesFromComment) {
   processor.AppendComment("@SystemApi This is a system API");
 
   std::stringstream result;
-  processor.WriteToStream(&result, "");
+  processor.WriteToStream("", &result);
   std::string annotations = result.str();
 
   EXPECT_THAT(annotations, HasSubstr("@android.annotation.SystemApi"));
@@ -53,6 +53,19 @@ TEST(AnnotationProcessorTest, EmitsSystemApiAnnotationAndRemovesFromComment) {
   EXPECT_THAT(annotations, HasSubstr("This is a system API"));
 }
 
+TEST(AnnotationProcessorTest, EmitsTestApiAnnotationAndRemovesFromComment) {
+  AnnotationProcessor processor;
+  processor.AppendComment("@TestApi This is a test API");
+
+  std::stringstream result;
+  processor.WriteToStream("", &result);
+  std::string annotations = result.str();
+
+  EXPECT_THAT(annotations, HasSubstr("@android.annotation.TestApi"));
+  EXPECT_THAT(annotations, Not(HasSubstr("@TestApi")));
+  EXPECT_THAT(annotations, HasSubstr("This is a test API"));
+}
+
 TEST(AnnotationProcessor, ExtractsFirstSentence) {
   EXPECT_THAT(AnnotationProcessor::ExtractFirstSentence("This is the only sentence"),
               Eq("This is the only sentence"));
diff --git a/tools/aapt2/java/ClassDefinition.cpp b/tools/aapt2/java/ClassDefinition.cpp
index 0cec9ae407f5..45130a4d7681 100644
--- a/tools/aapt2/java/ClassDefinition.cpp
+++ b/tools/aapt2/java/ClassDefinition.cpp
@@ -18,12 +18,12 @@
 
 #include "androidfw/StringPiece.h"
 
-using android::StringPiece;
+using ::android::StringPiece;
 
 namespace aapt {
 
 void ClassMember::WriteToStream(const StringPiece& prefix, bool final, std::ostream* out) const {
-  processor_.WriteToStream(out, prefix);
+  processor_.WriteToStream(prefix, out);
 }
 
 void MethodDefinition::AppendStatement(const StringPiece& statement) {
@@ -81,9 +81,8 @@ constexpr static const char* sWarningHeader =
     " * should not be modified by hand.\n"
     " */\n\n";
 
-bool ClassDefinition::WriteJavaFile(const ClassDefinition* def,
-                                    const StringPiece& package, bool final,
-                                    std::ostream* out) {
+bool ClassDefinition::WriteJavaFile(const ClassDefinition* def, const StringPiece& package,
+                                    bool final, std::ostream* out) {
   *out << sWarningHeader << "package " << package << ";\n\n";
   def->WriteToStream("", final, out);
   return bool(*out);
diff --git a/tools/aapt2/java/JavaClassGenerator_test.cpp b/tools/aapt2/java/JavaClassGenerator_test.cpp
index 271279ff5e92..4f449f0db41a 100644
--- a/tools/aapt2/java/JavaClassGenerator_test.cpp
+++ b/tools/aapt2/java/JavaClassGenerator_test.cpp
@@ -22,7 +22,9 @@
 #include "test/Test.h"
 #include "util/Util.h"
 
-using android::StringPiece;
+using ::android::StringPiece;
+using ::testing::HasSubstr;
+using ::testing::Not;
 
 namespace aapt {
 
@@ -52,17 +54,15 @@ TEST(JavaClassGeneratorTest, TransformInvalidJavaIdentifierCharacter) {
           .AddSimple("android:id/hey-man", ResourceId(0x01020000))
           .AddValue("android:attr/cool.attr", ResourceId(0x01010000),
                     test::AttributeBuilder(false).Build())
-          .AddValue(
-              "android:styleable/hey.dude", ResourceId(0x01030000),
-              test::StyleableBuilder()
-                  .AddItem("android:attr/cool.attr", ResourceId(0x01010000))
-                  .Build())
+          .AddValue("android:styleable/hey.dude", ResourceId(0x01030000),
+                    test::StyleableBuilder()
+                        .AddItem("android:attr/cool.attr", ResourceId(0x01010000))
+                        .Build())
           .Build();
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
   JavaClassGenerator generator(context.get(), table.get(), {});
@@ -72,14 +72,9 @@ TEST(JavaClassGeneratorTest, TransformInvalidJavaIdentifierCharacter) {
 
   std::string output = out.str();
 
-  EXPECT_NE(std::string::npos,
-            output.find("public static final int hey_man=0x01020000;"));
-
-  EXPECT_NE(std::string::npos,
-            output.find("public static final int[] hey_dude={"));
-
-  EXPECT_NE(std::string::npos,
-            output.find("public static final int hey_dude_cool_attr=0;"));
+  EXPECT_THAT(output, HasSubstr("public static final int hey_man=0x01020000;"));
+  EXPECT_THAT(output, HasSubstr("public static final int[] hey_dude={"));
+  EXPECT_THAT(output, HasSubstr("public static final int hey_dude_cool_attr=0;"));
 }
 
 TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
@@ -92,8 +87,7 @@ TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
   JavaClassGenerator generator(context.get(), table.get(), {});
@@ -101,11 +95,10 @@ TEST(JavaClassGeneratorTest, CorrectPackageNameIsUsed) {
   ASSERT_TRUE(generator.Generate("android", "com.android.internal", &out));
 
   std::string output = out.str();
-  EXPECT_NE(std::string::npos, output.find("package com.android.internal;"));
-  EXPECT_NE(std::string::npos,
-            output.find("public static final int one=0x01020000;"));
-  EXPECT_EQ(std::string::npos, output.find("two"));
-  EXPECT_EQ(std::string::npos, output.find("com_foo$two"));
+  EXPECT_THAT(output, HasSubstr("package com.android.internal;"));
+  EXPECT_THAT(output, HasSubstr("public static final int one=0x01020000;"));
+  EXPECT_THAT(output, Not(HasSubstr("two")));
+  EXPECT_THAT(output, Not(HasSubstr("com_foo$two")));
 }
 
 TEST(JavaClassGeneratorTest, AttrPrivateIsWrittenAsAttr) {
@@ -118,8 +111,7 @@ TEST(JavaClassGeneratorTest, AttrPrivateIsWrittenAsAttr) {
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
   JavaClassGenerator generator(context.get(), table.get(), {});
@@ -127,9 +119,8 @@ TEST(JavaClassGeneratorTest, AttrPrivateIsWrittenAsAttr) {
   ASSERT_TRUE(generator.Generate("android", &out));
 
   std::string output = out.str();
-  EXPECT_NE(std::string::npos, output.find("public static final class attr"));
-  EXPECT_EQ(std::string::npos,
-            output.find("public static final class ^attr-private"));
+  EXPECT_THAT(output, HasSubstr("public static final class attr"));
+  EXPECT_THAT(output, Not(HasSubstr("public static final class ^attr-private")));
 }
 
 TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
@@ -140,16 +131,13 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
           .AddSimple("android:id/one", ResourceId(0x01020000))
           .AddSimple("android:id/two", ResourceId(0x01020001))
           .AddSimple("android:id/three", ResourceId(0x01020002))
-          .SetSymbolState("android:id/one", ResourceId(0x01020000),
-                          SymbolState::kPublic)
-          .SetSymbolState("android:id/two", ResourceId(0x01020001),
-                          SymbolState::kPrivate)
+          .SetSymbolState("android:id/one", ResourceId(0x01020000), SymbolState::kPublic)
+          .SetSymbolState("android:id/two", ResourceId(0x01020001), SymbolState::kPrivate)
           .Build();
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
 
@@ -160,10 +148,9 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
     std::stringstream out;
     ASSERT_TRUE(generator.Generate("android", &out));
     std::string output = out.str();
-    EXPECT_NE(std::string::npos,
-              output.find("public static final int one=0x01020000;"));
-    EXPECT_EQ(std::string::npos, output.find("two"));
-    EXPECT_EQ(std::string::npos, output.find("three"));
+    EXPECT_THAT(output, HasSubstr("public static final int one=0x01020000;"));
+    EXPECT_THAT(output, Not(HasSubstr("two")));
+    EXPECT_THAT(output, Not(HasSubstr("three")));
   }
 
   options.types = JavaClassGeneratorOptions::SymbolTypes::kPublicPrivate;
@@ -172,11 +159,9 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
     std::stringstream out;
     ASSERT_TRUE(generator.Generate("android", &out));
     std::string output = out.str();
-    EXPECT_NE(std::string::npos,
-              output.find("public static final int one=0x01020000;"));
-    EXPECT_NE(std::string::npos,
-              output.find("public static final int two=0x01020001;"));
-    EXPECT_EQ(std::string::npos, output.find("three"));
+    EXPECT_THAT(output, HasSubstr("public static final int one=0x01020000;"));
+    EXPECT_THAT(output, HasSubstr("public static final int two=0x01020001;"));
+    EXPECT_THAT(output, Not(HasSubstr("three")));
   }
 
   options.types = JavaClassGeneratorOptions::SymbolTypes::kAll;
@@ -185,12 +170,9 @@ TEST(JavaClassGeneratorTest, OnlyWritePublicResources) {
     std::stringstream out;
     ASSERT_TRUE(generator.Generate("android", &out));
     std::string output = out.str();
-    EXPECT_NE(std::string::npos,
-              output.find("public static final int one=0x01020000;"));
-    EXPECT_NE(std::string::npos,
-              output.find("public static final int two=0x01020001;"));
-    EXPECT_NE(std::string::npos,
-              output.find("public static final int three=0x01020002;"));
+    EXPECT_THAT(output, HasSubstr("public static final int one=0x01020000;"));
+    EXPECT_THAT(output, HasSubstr("public static final int two=0x01020001;"));
+    EXPECT_THAT(output, HasSubstr("public static final int three=0x01020002;"));
   }
 }
 
@@ -246,8 +228,7 @@ TEST(JavaClassGeneratorTest, EmitOtherPackagesAttributesInStyleable) {
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
   JavaClassGenerator generator(context.get(), table.get(), {});
@@ -256,8 +237,8 @@ TEST(JavaClassGeneratorTest, EmitOtherPackagesAttributesInStyleable) {
   EXPECT_TRUE(generator.Generate("android", &out));
 
   std::string output = out.str();
-  EXPECT_NE(std::string::npos, output.find("int foo_bar="));
-  EXPECT_NE(std::string::npos, output.find("int foo_com_lib_bar="));
+  EXPECT_THAT(output, HasSubstr("int foo_bar="));
+  EXPECT_THAT(output, HasSubstr("int foo_com_lib_bar="));
 }
 
 TEST(JavaClassGeneratorTest, CommentsForSimpleResourcesArePresent) {
@@ -271,24 +252,22 @@ TEST(JavaClassGeneratorTest, CommentsForSimpleResourcesArePresent) {
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
   JavaClassGenerator generator(context.get(), table.get(), {});
   std::stringstream out;
   ASSERT_TRUE(generator.Generate("android", &out));
-  std::string actual = out.str();
+  std::string output = out.str();
 
-  const char* expectedText =
+  const char* expected_text =
       R"EOF(/**
      * This is a comment
      * @deprecated
      */
     @Deprecated
     public static final int foo=0x01010000;)EOF";
-
-  EXPECT_NE(std::string::npos, actual.find(expectedText));
+  EXPECT_THAT(output, HasSubstr(expected_text));
 }
 
 TEST(JavaClassGeneratorTest, CommentsForEnumAndFlagAttributesArePresent) {}
@@ -298,8 +277,7 @@ TEST(JavaClassGeneratorTest, CommentsForStyleablesAndNestedAttributesArePresent)
   attr.SetComment(StringPiece("This is an attribute"));
 
   Styleable styleable;
-  styleable.entries.push_back(
-      Reference(test::ParseNameOrDie("android:attr/one")));
+  styleable.entries.push_back(Reference(test::ParseNameOrDie("android:attr/one")));
   styleable.SetComment(StringPiece("This is a styleable"));
 
   std::unique_ptr<ResourceTable> table =
@@ -312,8 +290,7 @@ TEST(JavaClassGeneratorTest, CommentsForStyleablesAndNestedAttributesArePresent)
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
   JavaClassGeneratorOptions options;
@@ -321,12 +298,12 @@ TEST(JavaClassGeneratorTest, CommentsForStyleablesAndNestedAttributesArePresent)
   JavaClassGenerator generator(context.get(), table.get(), options);
   std::stringstream out;
   ASSERT_TRUE(generator.Generate("android", &out));
-  std::string actual = out.str();
+  std::string output = out.str();
 
-  EXPECT_NE(std::string::npos, actual.find("attr name android:one"));
-  EXPECT_NE(std::string::npos, actual.find("attr description"));
-  EXPECT_NE(std::string::npos, actual.find(attr.GetComment().data()));
-  EXPECT_NE(std::string::npos, actual.find(styleable.GetComment().data()));
+  EXPECT_THAT(output, HasSubstr("attr name android:one"));
+  EXPECT_THAT(output, HasSubstr("attr description"));
+  EXPECT_THAT(output, HasSubstr(attr.GetComment()));
+  EXPECT_THAT(output, HasSubstr(styleable.GetComment()));
 }
 
 TEST(JavaClassGeneratorTest, CommentsForRemovedAttributesAreNotPresentInClass) {
@@ -341,8 +318,7 @@ TEST(JavaClassGeneratorTest, CommentsForRemovedAttributesAreNotPresentInClass) {
 
   std::unique_ptr<IAaptContext> context =
       test::ContextBuilder()
-          .AddSymbolSource(
-              util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
           .SetNameManglerPolicy(NameManglerPolicy{"android"})
           .Build();
   JavaClassGeneratorOptions options;
@@ -350,17 +326,17 @@ TEST(JavaClassGeneratorTest, CommentsForRemovedAttributesAreNotPresentInClass) {
   JavaClassGenerator generator(context.get(), table.get(), options);
   std::stringstream out;
   ASSERT_TRUE(generator.Generate("android", &out));
-  std::string actual = out.str();
+  std::string output = out.str();
 
-  EXPECT_EQ(std::string::npos, actual.find("@attr name android:one"));
-  EXPECT_EQ(std::string::npos, actual.find("@attr description"));
+  EXPECT_THAT(output, Not(HasSubstr("@attr name android:one")));
+  EXPECT_THAT(output, Not(HasSubstr("@attr description")));
 
   // We should find @removed only in the attribute javadoc and not anywhere else
-  // (i.e. the class
-  // javadoc).
-  const size_t pos = actual.find("removed");
-  EXPECT_NE(std::string::npos, pos);
-  EXPECT_EQ(std::string::npos, actual.find("removed", pos + 1));
+  // (i.e. the class javadoc).
+  const std::string kRemoved("removed");
+  ASSERT_THAT(output, HasSubstr(kRemoved));
+  std::string after_first_match = output.substr(output.find(kRemoved) + kRemoved.size());
+  EXPECT_THAT(after_first_match, Not(HasSubstr(kRemoved)));
 }
 
 TEST(JavaClassGeneratorTest, GenerateOnResourcesLoadedCallbackForSharedLibrary) {
@@ -381,19 +357,17 @@ TEST(JavaClassGeneratorTest, GenerateOnResourcesLoadedCallbackForSharedLibrary)
 
   JavaClassGeneratorOptions options;
   options.use_final = false;
-  options.rewrite_callback_options = OnResourcesLoadedCallbackOptions{
-      {"com.foo", "com.boo"},
-  };
+  options.rewrite_callback_options = OnResourcesLoadedCallbackOptions{{"com.foo", "com.boo"}};
   JavaClassGenerator generator(context.get(), table.get(), options);
 
   std::stringstream out;
   ASSERT_TRUE(generator.Generate("android", &out));
 
-  std::string actual = out.str();
+  std::string output = out.str();
 
-  EXPECT_NE(std::string::npos, actual.find("void onResourcesLoaded"));
-  EXPECT_NE(std::string::npos, actual.find("com.foo.R.onResourcesLoaded"));
-  EXPECT_NE(std::string::npos, actual.find("com.boo.R.onResourcesLoaded"));
+  EXPECT_THAT(output, HasSubstr("void onResourcesLoaded"));
+  EXPECT_THAT(output, HasSubstr("com.foo.R.onResourcesLoaded"));
+  EXPECT_THAT(output, HasSubstr("com.boo.R.onResourcesLoaded"));
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/java/ManifestClassGenerator.cpp b/tools/aapt2/java/ManifestClassGenerator.cpp
index f49e4985fcf1..4ef32c92dd84 100644
--- a/tools/aapt2/java/ManifestClassGenerator.cpp
+++ b/tools/aapt2/java/ManifestClassGenerator.cpp
@@ -21,24 +21,21 @@
 #include "Source.h"
 #include "java/AnnotationProcessor.h"
 #include "java/ClassDefinition.h"
+#include "text/Unicode.h"
 #include "util/Maybe.h"
 #include "xml/XmlDom.h"
 
-using android::StringPiece;
+using ::android::StringPiece;
+using ::aapt::text::IsJavaIdentifier;
 
 namespace aapt {
 
-static Maybe<StringPiece> ExtractJavaIdentifier(IDiagnostics* diag,
-                                                const Source& source,
-                                                const StringPiece& value) {
-  const StringPiece sep = ".";
-  auto iter = std::find_end(value.begin(), value.end(), sep.begin(), sep.end());
-
-  StringPiece result;
-  if (iter != value.end()) {
-    result.assign(iter + sep.size(), value.end() - (iter + sep.size()));
-  } else {
-    result = value;
+static Maybe<StringPiece> ExtractJavaIdentifier(IDiagnostics* diag, const Source& source,
+                                                const std::string& value) {
+  StringPiece result = value;
+  size_t pos = value.rfind('.');
+  if (pos != std::string::npos) {
+    result = result.substr(pos + 1);
   }
 
   if (result.empty()) {
@@ -46,33 +43,23 @@ static Maybe<StringPiece> ExtractJavaIdentifier(IDiagnostics* diag,
     return {};
   }
 
-  iter = util::FindNonAlphaNumericAndNotInSet(result, "_");
-  if (iter != result.end()) {
-    diag->Error(DiagMessage(source) << "invalid character '"
-                                    << StringPiece(iter, 1) << "' in '"
-                                    << result << "'");
+  if (!IsJavaIdentifier(result)) {
+    diag->Error(DiagMessage(source) << "invalid Java identifier '" << result << "'");
     return {};
   }
-
-  if (*result.begin() >= '0' && *result.begin() <= '9') {
-    diag->Error(DiagMessage(source) << "symbol can not start with a digit");
-    return {};
-  }
-
   return result;
 }
 
-static bool WriteSymbol(const Source& source, IDiagnostics* diag,
-                        xml::Element* el, ClassDefinition* class_def) {
+static bool WriteSymbol(const Source& source, IDiagnostics* diag, xml::Element* el,
+                        ClassDefinition* class_def) {
   xml::Attribute* attr = el->FindAttribute(xml::kSchemaAndroid, "name");
   if (!attr) {
-    diag->Error(DiagMessage(source) << "<" << el->name
-                                    << "> must define 'android:name'");
+    diag->Error(DiagMessage(source) << "<" << el->name << "> must define 'android:name'");
     return false;
   }
 
-  Maybe<StringPiece> result = ExtractJavaIdentifier(
-      diag, source.WithLine(el->line_number), attr->value);
+  Maybe<StringPiece> result =
+      ExtractJavaIdentifier(diag, source.WithLine(el->line_number), attr->value);
   if (!result) {
     return false;
   }
@@ -85,8 +72,7 @@ static bool WriteSymbol(const Source& source, IDiagnostics* diag,
   return true;
 }
 
-std::unique_ptr<ClassDefinition> GenerateManifestClass(IDiagnostics* diag,
-                                                       xml::XmlResource* res) {
+std::unique_ptr<ClassDefinition> GenerateManifestClass(IDiagnostics* diag, xml::XmlResource* res) {
   xml::Element* el = xml::FindRootElement(res->root.get());
   if (!el) {
     diag->Error(DiagMessage(res->file.source) << "no root tag defined");
@@ -94,8 +80,7 @@ std::unique_ptr<ClassDefinition> GenerateManifestClass(IDiagnostics* diag,
   }
 
   if (el->name != "manifest" && !el->namespace_uri.empty()) {
-    diag->Error(DiagMessage(res->file.source)
-                << "no <manifest> root tag defined");
+    diag->Error(DiagMessage(res->file.source) << "no <manifest> root tag defined");
     return {};
   }
 
@@ -109,11 +94,9 @@ std::unique_ptr<ClassDefinition> GenerateManifestClass(IDiagnostics* diag,
   for (xml::Element* child_el : children) {
     if (child_el->namespace_uri.empty()) {
       if (child_el->name == "permission") {
-        error |= !WriteSymbol(res->file.source, diag, child_el,
-                              permission_class.get());
+        error |= !WriteSymbol(res->file.source, diag, child_el, permission_class.get());
       } else if (child_el->name == "permission-group") {
-        error |= !WriteSymbol(res->file.source, diag, child_el,
-                              permission_group_class.get());
+        error |= !WriteSymbol(res->file.source, diag, child_el, permission_group_class.get());
       }
     }
   }
diff --git a/tools/aapt2/java/ManifestClassGenerator_test.cpp b/tools/aapt2/java/ManifestClassGenerator_test.cpp
index 9f6ec210a6a7..c744e9bd4fc9 100644
--- a/tools/aapt2/java/ManifestClassGenerator_test.cpp
+++ b/tools/aapt2/java/ManifestClassGenerator_test.cpp
@@ -84,6 +84,8 @@ TEST(ManifestClassGeneratorTest, CommentsAndAnnotationsArePresent) {
              @hide
              @SystemApi -->
         <permission android:name="android.permission.SECRET" />
+        <!-- @TestApi This is a test only permission. -->
+        <permission android:name="android.permission.TEST_ONLY" />
       </manifest>)");
 
   std::string actual;
@@ -110,6 +112,13 @@ TEST(ManifestClassGeneratorTest, CommentsAndAnnotationsArePresent) {
     @android.annotation.SystemApi
     public static final String SECRET="android.permission.SECRET";)";
   EXPECT_THAT(actual, HasSubstr(expected_secret));
+
+  const char* expected_test = R"(    /**
+     * This is a test only permission.
+     */
+    @android.annotation.TestApi
+    public static final String TEST_ONLY="android.permission.TEST_ONLY";)";
+  EXPECT_THAT(actual, HasSubstr(expected_test));
 }
 
 static ::testing::AssertionResult GetManifestClassText(IAaptContext* context, xml::XmlResource* res,