Address feedback from @joehan and go/atad-cli-api-review

firebase · Feb 6, 2024 · 9469c64 · 9469c64
1 parent 8042bbc
commit 9469c64
Show file tree

Hide file tree

Showing 5 changed files with 140 additions and 133 deletions.
diff --git a/src/appdistribution/client.ts b/src/appdistribution/client.ts
@@ -6,113 +6,16 @@ import { Distribution } from "./distribution";
 import { FirebaseError } from "../error";
 import { Client } from "../apiv2";
 import { appDistributionOrigin } from "../api";
-
-/**
- * Helper interface for an app that is provisioned with App Distribution
- */
-export interface AabInfo {
-  name: string;
-  integrationState: IntegrationState;
-  testCertificate: TestCertificate | null;
-}
-
-export interface TestCertificate {
-  hashSha1: string;
-  hashSha256: string;
-  hashMd5: string;
-}
-
-/** Enum representing the App Bundles state for the App */
-export enum IntegrationState {
-  AAB_INTEGRATION_STATE_UNSPECIFIED = "AAB_INTEGRATION_STATE_UNSPECIFIED",
-  INTEGRATED = "INTEGRATED",
-  PLAY_ACCOUNT_NOT_LINKED = "PLAY_ACCOUNT_NOT_LINKED",
-  NO_APP_WITH_GIVEN_BUNDLE_ID_IN_PLAY_ACCOUNT = "NO_APP_WITH_GIVEN_BUNDLE_ID_IN_PLAY_ACCOUNT",
-  APP_NOT_PUBLISHED = "APP_NOT_PUBLISHED",
-  AAB_STATE_UNAVAILABLE = "AAB_STATE_UNAVAILABLE",
-  PLAY_IAS_TERMS_NOT_ACCEPTED = "PLAY_IAS_TERMS_NOT_ACCEPTED",
-}
-
-export enum UploadReleaseResult {
-  UPLOAD_RELEASE_RESULT_UNSPECIFIED = "UPLOAD_RELEASE_RESULT_UNSPECIFIED",
-  RELEASE_CREATED = "RELEASE_CREATED",
-  RELEASE_UPDATED = "RELEASE_UPDATED",
-  RELEASE_UNMODIFIED = "RELEASE_UNMODIFIED",
-}
-
-export interface Release {
-  name: string;
-  releaseNotes: ReleaseNotes;
-  displayVersion: string;
-  buildVersion: string;
-  createTime: Date;
-  firebaseConsoleUri: string;
-  testingUri: string;
-  binaryDownloadUri: string;
-}
-
-export interface ReleaseNotes {
-  text: string;
-}
-
-export interface UploadReleaseResponse {
-  result: UploadReleaseResult;
-  release: Release;
-}
-
-export interface BatchRemoveTestersResponse {
-  emails: string[];
-}
-
-export interface Group {
-  name: string;
-  displayName: string;
-  testerCount?: number;
-  releaseCount?: number;
-  inviteLinkCount?: number;
-}
-
-export interface CreateReleaseTestRequest {
-  releaseTest: ReleaseTest;
-}
-
-export enum TestState {
-  IN_PROGRESS = "IN_PROGRESS",
-  PASSED = "PASSED",
-  FAILED = "FAILED",
-  INCONCLUSIVE = "INCONCLUSIVE",
-}
-
-export interface TestDevice {
-  model: string;
-  version: string;
-  locale: string;
-  orientation: string;
-}
-
-export interface DeviceExecution {
-  device: TestDevice;
-  state?: TestState;
-  failedReason?: string;
-  inconclusiveReason?: string;
-}
-
-export interface FieldHints {
-  usernameResourceName?: string;
-  passwordResourceName?: string;
-}
-
-export interface LoginCredential {
-  username?: string;
-  password?: string;
-  fieldHints?: FieldHints;
-}
-
-export interface ReleaseTest {
-  name?: string;
-  deviceExecutions: DeviceExecution[];
-  loginCredential?: LoginCredential;
-}
+import {
+  AabInfo,
+  BatchRemoveTestersResponse,
+  DeviceExecution,
+  Group,
+  LoginCredential,
+  ReleaseTest,
+  TestDevice,
+  UploadReleaseResponse,
+} from "./types";
 
 /**
  * Makes RPCs to the App Distribution server backend.
@@ -321,7 +224,6 @@ export class AppDistributionClient {
           loginCredential,
         },
       });
-      utils.logSuccess(`Release test created successfully`);
       return response.body;
     } catch (err: any) {
       throw new FirebaseError(`Failed to create release test ${err}`);

diff --git a/src/appdistribution/options-parser-util.ts b/src/appdistribution/options-parser-util.ts
@@ -1,7 +1,7 @@
 import * as fs from "fs-extra";
 import { FirebaseError } from "../error";
 import { needProjectNumber } from "../projectUtils";
-import { FieldHints, LoginCredential, TestDevice } from "./client";
+import { FieldHints, LoginCredential, TestDevice } from "./types";
 
 /**
  * Takes in comma separated string or a path to a comma/new line separated file
@@ -81,13 +81,11 @@ export function getTestDevices(value: string, file: string): TestDevice[] {
     return [];
   }
 
-  // The value is split into a string[]
-  const deviceStrings = value
+  return value
     .split(/[;\n]/)
     .map((entry) => entry.trim())
-    .filter((entry) => !!entry);
-
-  return deviceStrings.map((str) => parseTestDevice(str));
+    .filter((entry) => !!entry)
+    .map((str) => parseTestDevice(str));
 }
 
 function parseTestDevice(testDeviceString: string): TestDevice {
@@ -134,9 +132,15 @@ function parseTestDevice(testDeviceString: string): TestDevice {
 export function getLoginCredential(
   username?: string,
   password?: string,
+  passwordFile?: string,
   usernameResourceName?: string,
   passwordResourceName?: string,
 ): LoginCredential | undefined {
+  if (!password && passwordFile) {
+    ensureFileExists(passwordFile);
+    password = fs.readFileSync(passwordFile, "utf8");
+  }
+
   if (isPresenceMismatched(usernameResourceName, passwordResourceName)) {
     throw new FirebaseError(
       "Username and password resource names for automated tests need to be specified together.",

diff --git a/src/appdistribution/types.ts b/src/appdistribution/types.ts
@@ -0,0 +1,101 @@
+/**
+ * Helper interface for an app that is provisioned with App Distribution
+ */
+export interface AabInfo {
+  name: string;
+  integrationState: IntegrationState;
+  testCertificate: TestCertificate | null;
+}
+
+export interface TestCertificate {
+  hashSha1: string;
+  hashSha256: string;
+  hashMd5: string;
+}
+
+/** Enum representing the App Bundles state for the App */
+export enum IntegrationState {
+  AAB_INTEGRATION_STATE_UNSPECIFIED = "AAB_INTEGRATION_STATE_UNSPECIFIED",
+  INTEGRATED = "INTEGRATED",
+  PLAY_ACCOUNT_NOT_LINKED = "PLAY_ACCOUNT_NOT_LINKED",
+  NO_APP_WITH_GIVEN_BUNDLE_ID_IN_PLAY_ACCOUNT = "NO_APP_WITH_GIVEN_BUNDLE_ID_IN_PLAY_ACCOUNT",
+  APP_NOT_PUBLISHED = "APP_NOT_PUBLISHED",
+  AAB_STATE_UNAVAILABLE = "AAB_STATE_UNAVAILABLE",
+  PLAY_IAS_TERMS_NOT_ACCEPTED = "PLAY_IAS_TERMS_NOT_ACCEPTED",
+}
+
+export enum UploadReleaseResult {
+  UPLOAD_RELEASE_RESULT_UNSPECIFIED = "UPLOAD_RELEASE_RESULT_UNSPECIFIED",
+  RELEASE_CREATED = "RELEASE_CREATED",
+  RELEASE_UPDATED = "RELEASE_UPDATED",
+  RELEASE_UNMODIFIED = "RELEASE_UNMODIFIED",
+}
+
+export interface Release {
+  name: string;
+  releaseNotes: ReleaseNotes;
+  displayVersion: string;
+  buildVersion: string;
+  createTime: Date;
+  firebaseConsoleUri: string;
+  testingUri: string;
+  binaryDownloadUri: string;
+}
+
+export interface ReleaseNotes {
+  text: string;
+}
+
+export interface UploadReleaseResponse {
+  result: UploadReleaseResult;
+  release: Release;
+}
+
+export interface BatchRemoveTestersResponse {
+  emails: string[];
+}
+
+export interface Group {
+  name: string;
+  displayName: string;
+  testerCount?: number;
+  releaseCount?: number;
+  inviteLinkCount?: number;
+}
+
+export interface CreateReleaseTestRequest {
+  releaseTest: ReleaseTest;
+}
+
+export interface TestDevice {
+  model: string;
+  version: string;
+  locale: string;
+  orientation: string;
+}
+
+export type TestState = "IN_PROGRESS" | "PASSED" | "FAILED" | "INCONCLUSIVE";
+
+export interface DeviceExecution {
+  device: TestDevice;
+  state?: TestState;
+  failedReason?: string;
+  inconclusiveReason?: string;
+}
+
+export interface FieldHints {
+  usernameResourceName?: string;
+  passwordResourceName?: string;
+}
+
+export interface LoginCredential {
+  username?: string;
+  password?: string;
+  fieldHints?: FieldHints;
+}
+
+export interface ReleaseTest {
+  name?: string;
+  deviceExecutions: DeviceExecution[];
+  loginCredential?: LoginCredential;
+}
diff --git a/src/commands/appdistribution-distribute.ts b/src/commands/appdistribution-distribute.ts
@@ -3,14 +3,13 @@ import * as fs from "fs-extra";
 import { Command } from "../command";
 import * as utils from "../utils";
 import { requireAuth } from "../requireAuth";
+import { AppDistributionClient } from "../appdistribution/client";
 import {
   AabInfo,
   IntegrationState,
-  AppDistributionClient,
   UploadReleaseResult,
-  TestState,
   TestDevice,
-} from "../appdistribution/client";
+} from "../appdistribution/types";
 import { FirebaseError } from "../error";
 import { Distribution, DistributionFileType } from "../appdistribution/distribution";
 import {
@@ -59,7 +58,11 @@ export const command = new Command("appdistribution:distribute <release-binary-f
     "path to file containing a list of semicolon- or newline-separated devices to run automated tests on, in the format 'model=<model-id>,version=<os-version-id>,locale=<locale>,orientation=<orientation>'. Run 'gcloud firebase test android|ios models list' to see available devices. Note: This feature is in beta.",
   )
   .option("--test-username <string>", "username for automatic login")
-  .option("--test-password <string>", "password for automatic login")
+  .option(
+    "--test-password <string>",
+    "password for automatic login. If using a real password, use --test-password-file instead to avoid putting sensitive info in history and logs.",
+  )
+  .option("--test-password-file <string>", "path to file containing password for automatic login")
   .option(
     "--test-username-resource <string>",
     "resource name for the username field for automatic login",
@@ -83,6 +86,7 @@ export const command = new Command("appdistribution:distribute <release-binary-f
     const loginCredential = getLoginCredential(
       options.testUsername,
       options.testPassword,
+      options.testPasswordFile,
       options.testUsernameResource,
       options.testPasswordResource,
     );
@@ -213,6 +217,7 @@ export const command = new Command("appdistribution:distribute <release-binary-f
         testDevices,
         loginCredential,
       );
+      utils.logSuccess(`Release test created successfully`);
       if (!options.testAsync) {
         await awaitTestResults(releaseTest.name!, requests);
       }
@@ -227,21 +232,21 @@ async function awaitTestResults(
     utils.logBullet("the automated tests results are pending");
     await delay(TEST_POLLING_INTERVAL_MILLIS);
     const releaseTest = await requests.getReleaseTest(releaseTestName);
-    if (releaseTest.deviceExecutions.every((e) => e.state === TestState.PASSED)) {
+    if (releaseTest.deviceExecutions.every((e) => e.state === "PASSED")) {
       utils.logSuccess("automated test(s) passed!");
       return;
     }
     for (const execution of releaseTest.deviceExecutions) {
       switch (execution.state) {
-        case TestState.PASSED:
-        case TestState.IN_PROGRESS:
+        case "PASSED":
+        case "IN_PROGRESS":
           continue;
-        case TestState.FAILED:
+        case "FAILED":
           throw new FirebaseError(
             `Automated test failed for ${deviceToString(execution.device)}: ${execution.failedReason}`,
             { exit: 1 },
           );
-        case TestState.INCONCLUSIVE:
+        case "INCONCLUSIVE":
           throw new FirebaseError(
             `Automated test inconclusive for ${deviceToString(execution.device)}: ${execution.inconclusiveReason}`,
             { exit: 1 },
@@ -259,7 +264,7 @@ async function awaitTestResults(
   });
 }
 
-function delay(ms: number) {
+function delay(ms: number): Promise<number> {
   return new Promise((resolve) => setTimeout(resolve, ms));
 }
 

diff --git a/src/test/appdistro/client.spec.ts b/src/test/appdistro/client.spec.ts
@@ -6,13 +6,8 @@ import * as rimraf from "rimraf";
 import * as sinon from "sinon";
 import * as tmp from "tmp";
 
-import {
-  AppDistributionClient,
-  BatchRemoveTestersResponse,
-  Group,
-  TestDevice,
-  TestState,
-} from "../../appdistribution/client";
+import { AppDistributionClient } from "../../appdistribution/client";
+import { BatchRemoveTestersResponse, Group, TestDevice } from "../../appdistribution/types";
 import { appDistributionOrigin } from "../../api";
 import { Distribution } from "../../appdistribution/distribution";
 import { FirebaseError } from "../../error";
@@ -322,7 +317,7 @@ describe("distribution", () => {
     const mockReleaseTest = {
       name: `${releaseName}/tests/fake-test-id`,
       devices: mockDevices,
-      state: TestState.IN_PROGRESS,
+      state: "IN_PROGRESS",
     };
 
     it("should throw error if request fails", async () => {
@@ -363,7 +358,7 @@ describe("distribution", () => {
     const mockReleaseTest = {
       name: releaseTestName,
       devices: mockDevices,
-      state: TestState.IN_PROGRESS,
+      state: "IN_PROGRESS",
     };
 
     it("should throw error if request fails", async () => {