Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions src/appdistribution/distribution.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,18 +114,19 @@
],
});

const setTimeoutStub = sinon.stub(global, "setTimeout").callsFake((fn) => fn() as any);

Check warning on line 117 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type

Check warning on line 117 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe return of an `any` typed value

await awaitTestResults(releaseTests, mockClient as any);

Check warning on line 119 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type

Check warning on line 119 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe argument of type `any` assigned to a parameter of type `AppDistributionClient`

expect(logSuccessStub).to.have.been.calledWithMatch(/Automated test\(s\) passed/);
setTimeoutStub.restore();
});

it("should fail immediately when a test execution fails", async () => {
it("should fail and report failed count when a test execution fails", async () => {
const releaseTests: ReleaseTest[] = [{ name: "tests/1", deviceExecutions: [] }];
mockClient.getReleaseTest.resolves({

Check warning on line 127 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe argument of type `any` assigned to a parameter of type `ReleaseTest | undefined`
name: "tests/1",
displayName: "Login Case",
deviceExecutions: [
{
state: "FAILED",
Expand All @@ -133,19 +134,50 @@
device: { model: "Pixel", version: "14", locale: "en_US", orientation: "PORTRAIT" },
},
],
} as any);

Check warning on line 137 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type

const setTimeoutStub = sinon.stub(global, "setTimeout").callsFake((fn) => fn() as any);

Check warning on line 139 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type

Check warning on line 139 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe return of an `any` typed value

let caughtError: any;

Check warning on line 141 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected any. Specify a different type
try {
await awaitTestResults(releaseTests, mockClient as any);

Check warning on line 143 in src/appdistribution/distribution.spec.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unsafe argument of type `any` assigned to a parameter of type `AppDistributionClient`
} catch (err) {
caughtError = err;
}

expect(caughtError).to.exist;
expect(caughtError.message).to.match(/Automated test failed/);
expect(caughtError.message).to.match(/Automated test\(s\) failed/);
setTimeoutStub.restore();
});

it("should write machine-readable JSON results file when specified", async () => {
const outputJsonStub = sinon.stub(fs, "outputJsonSync");
const releaseTests: ReleaseTest[] = [{ name: "tests/1", deviceExecutions: [] }];
mockClient.getReleaseTest.resolves({
name: "tests/1",
displayName: "Login Case",
resultsBucket: "projects/123/buckets/my-bucket",
deviceExecutions: [
{
state: "PASSED",
device: { model: "Pixel", version: "14", locale: "en_US", orientation: "PORTRAIT" },
},
],
} as any);

const setTimeoutStub = sinon.stub(global, "setTimeout").callsFake((fn) => fn() as any);

await awaitTestResults(releaseTests, mockClient as any, "/tmp/results.json");

expect(outputJsonStub).to.have.been.calledWith(
"/tmp/results.json",
sinon.match({
passed: true,
totalTestCases: 1,
passCount: 1,
failCount: 0,
}),
);
setTimeoutStub.restore();
});
});
Expand Down
193 changes: 159 additions & 34 deletions src/appdistribution/distribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,56 +120,181 @@ export class Distribution {
}
}

export interface DeviceExecutionResult {
device: string;
state: string;
failedReason?: string;
inconclusiveReason?: string;
actionsJsonGcsUri?: string;
}

export interface TestCaseResult {
displayName: string;
releaseTestName?: string;
passed: boolean;
deviceExecutions: DeviceExecutionResult[];
}

export interface TestResultSummary {
passed: boolean;
totalTestCases: number;
passCount: number;
failCount: number;
testCases: TestCaseResult[];
}

/** Wait for release tests to complete */
export async function awaitTestResults(
releaseTests: ReleaseTest[],
requests: AppDistributionClient,
): Promise<void> {
const releaseTestNames = new Set(
resultsFilePath?: string,
): Promise<TestResultSummary> {
const pendingReleaseTestNames = new Set(
releaseTests.map((rt) => rt.name).filter((n): n is string => !!n),
);
const completedReleaseTests = new Map<string, ReleaseTest>();

for (let i = 0; i < TEST_MAX_POLLING_RETRIES; i++) {
utils.logBullet(`${releaseTestNames.size} automated test results are pending...`);
if (pendingReleaseTestNames.size === 0) {
break;
}
utils.logBullet(`${pendingReleaseTestNames.size} automated test(s) pending completion...`);
await delay(TEST_POLLING_INTERVAL_MILLIS);
for (const releaseTestName of releaseTestNames) {

for (const releaseTestName of Array.from(pendingReleaseTestNames)) {
const releaseTest = await requests.getReleaseTest(releaseTestName);
if (releaseTest.deviceExecutions.every((e) => e.state === "PASSED")) {
releaseTestNames.delete(releaseTestName);
if (releaseTestNames.size === 0) {
utils.logSuccess("Automated test(s) passed!");
return;
const isFinished = releaseTest.deviceExecutions.every(
(e) => e.state && e.state !== "IN_PROGRESS",
);
if (isFinished) {
pendingReleaseTestNames.delete(releaseTestName);
completedReleaseTests.set(releaseTestName, releaseTest);

const displayName = releaseTest.displayName || releaseTestName;
const allPassed = releaseTest.deviceExecutions.every((e) => e.state === "PASSED");
if (allPassed) {
const deviceList = releaseTest.deviceExecutions
.map((e) => deviceToString(e.device))
.join(", ");
utils.logSuccess(`✔ Passed: "${displayName}" (${deviceList})`);
} else {
continue;
const failedExecs = releaseTest.deviceExecutions.filter((e) => e.state !== "PASSED");
for (const exec of failedExecs) {
const devStr = deviceToString(exec.device);
const reason = exec.failedReason || exec.inconclusiveReason || exec.state || "Failed";
utils.logWarning(`✖ Failed: "${displayName}" on ${devStr}: ${reason}`);
const gcsUri = formatActionsGcsUri(
releaseTest.resultsBucket,
releaseTest.name,
exec.device.model,
);
if (gcsUri) {
logger.info(` Log / Actions detail: ${gcsUri}`);
}
}
}
}
Comment on lines +166 to 196

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

To adhere to the repository style guide on reducing nesting and to fix a potential bug with empty deviceExecutions, we should extract the test case result logging into a helper function and ensure we guard against empty deviceExecutions arrays.

Currently, if deviceExecutions is empty, every returns true, which would immediately mark the test as finished and passed, logging a success message. However, the final summary on line 214 requires deviceExecutions.length > 0 to pass, leading to contradictory console output (logging success but exiting with failure).

By extracting this logic, we reduce the nesting level from 6 to 4 and ensure consistent handling of empty executions.

      const isFinished =
        releaseTest.deviceExecutions.length > 0 &&
        releaseTest.deviceExecutions.every((e) => e.state && e.state !== "IN_PROGRESS");
      if (isFinished) {
        pendingReleaseTestNames.delete(releaseTestName);
        completedReleaseTests.set(releaseTestName, releaseTest);
        logTestCaseResult(releaseTest, releaseTest.displayName || releaseTestName);
      }
References
  1. Reduce nesting as much as possible. Code should avoid unnecessarily deep nesting or long periods of nesting. Handle edge cases early and exit or fold them into the general case. Consider helper functions that can completely encapsulate branching. (link)

for (const execution of releaseTest.deviceExecutions) {
const device = deviceToString(execution.device);
switch (execution.state) {
case "PASSED":
case "IN_PROGRESS":
continue;
case "FAILED":
throw new FirebaseError(
`Automated test failed for ${device}: ${execution.failedReason}`,
{ exit: 1 },
);
case "INCONCLUSIVE":
throw new FirebaseError(
`Automated test inconclusive for ${device}: ${execution.inconclusiveReason}`,
{ exit: 1 },
);
default:
throw new FirebaseError(
`Unsupported automated test state for ${device}: ${execution.state}`,
{ exit: 1 },
);
}
}

if (pendingReleaseTestNames.size > 0) {
throw new FirebaseError("It took longer than expected to run your test(s), please try again.", {
exit: 1,
});
}

const summaryTestCases: TestCaseResult[] = [];
let totalFailures = 0;
let totalPasses = 0;

for (const rt of releaseTests) {
const name = rt.name;
const finalRt = (name && completedReleaseTests.get(name)) || rt;
const displayName = finalRt.displayName || name || "Test Case";
const casePassed =
finalRt.deviceExecutions.length > 0 &&
finalRt.deviceExecutions.every((e) => e.state === "PASSED");

if (casePassed) {
totalPasses++;
} else {
totalFailures++;
}

summaryTestCases.push({
displayName,
releaseTestName: name,
passed: casePassed,
deviceExecutions: finalRt.deviceExecutions.map((e) => ({
device: deviceToString(e.device),
state: e.state || "UNKNOWN",
failedReason: e.failedReason,
inconclusiveReason: e.inconclusiveReason,
actionsJsonGcsUri: formatActionsGcsUri(finalRt.resultsBucket, name, e.device.model),
})),
});
}

const summary: TestResultSummary = {
passed: totalFailures === 0,
totalTestCases: summaryTestCases.length,
passCount: totalPasses,
failCount: totalFailures,
testCases: summaryTestCases,
};

if (resultsFilePath) {
try {
fs.outputJsonSync(resultsFilePath, summary, { spaces: 2 });
utils.logSuccess(`Wrote machine-readable test results to ${resultsFilePath}`);
} catch (err: unknown) {
logger.info(`Failed to write results file to ${resultsFilePath}: ${getErrMsg(err)}`);
}
}

if (totalFailures > 0) {
utils.logWarning(
`\nAutomated test run finished with failures (${totalFailures} of ${summaryTestCases.length} test cases failed):`,
);
for (const tc of summaryTestCases) {
if (!tc.passed) {
utils.logWarning(` - Case "${tc.displayName}":`);
for (const exec of tc.deviceExecutions) {
if (exec.state !== "PASSED") {
const reason = exec.failedReason || exec.inconclusiveReason || exec.state;
utils.logWarning(` • ${exec.device}: ${reason}`);
if (exec.actionsJsonGcsUri) {
utils.logWarning(` GCS actions log: ${exec.actionsJsonGcsUri}`);
}
}
}
}
}
throw new FirebaseError(
`Automated test(s) failed: ${totalFailures} of ${summaryTestCases.length} test cases failed.`,
{ exit: 1 },
);
}

utils.logSuccess(
`Automated test(s) passed! (${totalPasses} of ${summaryTestCases.length} test cases passed)`,
);
return summary;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add the helper function logTestCaseResult to handle logging of completed test cases, ensuring that we also guard against empty deviceExecutions here.

}

function logTestCaseResult(releaseTest: ReleaseTest, displayName: string): void {
  const allPassed =
    releaseTest.deviceExecutions.length > 0 &&
    releaseTest.deviceExecutions.every((e) => e.state === "PASSED");
  if (allPassed) {
    const deviceList = releaseTest.deviceExecutions
      .map((e) => deviceToString(e.device))
      .join(", ");
    utils.logSuccess(`✔ Passed: "${displayName}" (${deviceList})`);
    return;
  }

  const failedExecs = releaseTest.deviceExecutions.filter((e) => e.state !== "PASSED");
  for (const exec of failedExecs) {
    const devStr = deviceToString(exec.device);
    const reason = exec.failedReason || exec.inconclusiveReason || exec.state || "Failed";
    utils.logWarning(`✖ Failed: "${displayName}" on ${devStr}: ${reason}`);
    const gcsUri = formatActionsGcsUri(
      releaseTest.resultsBucket,
      releaseTest.name,
      exec.device.model,
    );
    if (gcsUri) {
      logger.info(`   Log / Actions detail: ${gcsUri}`);
    }
  }
}


function formatActionsGcsUri(
resultsBucket: string | undefined,
releaseTestName: string | undefined,
model: string,
): string | undefined {
if (!resultsBucket || !releaseTestName) {
return undefined;
}
throw new FirebaseError("It took longer than expected to run your test(s), please try again.", {
exit: 1,
});
const bucketName = resultsBucket.includes("buckets/")
? resultsBucket.split("buckets/")[1]
: resultsBucket.replace(/^gs:\/\//, "");
const testId = releaseTestName.split("/").pop() || "";
return `gs://${bucketName}/${testId}/${model}/actions.json`;
}

function delay(ms: number): Promise<number> {
Expand Down
12 changes: 10 additions & 2 deletions src/commands/apptesting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ export const command = new Command("apptesting:execute [release-binary-file]")
"--results-bucket <bucket>",
"The name of a Google Cloud Storage bucket where raw test results will be stored. If this flag is not set, Firebase creates a default bucket for you. Note that the bucket must be owned by a billing-enabled project, and that using a non-default bucket will result in billing charges for the storage used.",
)
.option(
"--results-file <results_file>",
"Path to output a JSON file containing machine-readable test results.",
)
.option("--test-username <string>", "username for automatic login")
.option(
"--test-password <string>",
Expand Down Expand Up @@ -141,7 +145,7 @@ export const command = new Command("apptesting:execute [release-binary-file]")
`View progress and results in the Firebase Console:\n${release.firebaseConsoleUri}`,
);
} else {
await awaitTestResults(releaseTests, client);
await awaitTestResults(releaseTests, client, options.resultsFile);
utils.logBullet(
`View detailed results in the Firebase Console:\n${release.firebaseConsoleUri}`,
);
Expand Down Expand Up @@ -173,6 +177,10 @@ async function invokeTests(
}
return releaseTests;
} catch (err: unknown) {
throw new FirebaseError("Test invocation failed", { original: getError(err) });
const errObj = getError(err);
if (errObj instanceof FirebaseError) {
throw errObj;
}
throw new FirebaseError(`Test invocation failed: ${errObj.message}`, { original: errObj });
}
}
Loading