Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for AI Testing Agent #8001

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Support for AI Testing Agent #8001

wants to merge 2 commits into from

Conversation

kaibolay
Copy link
Contributor

Add support for --test-case-ids and --test-case-ids-file to appdistribution:distribute to launch .

Description

The Automated Testing Agent is a beta feature of App Distribution. Once test cases are created in the Firebase Console, then now can be launched automatically after uploading a new binary.

Scenarios Tested

Tested firebaseappdistribution:distribute with and without --test-case-ids flag.

Sample Commands

firebase firebaseappdistribution:distribute --app "1:987654321:android:deadbeef" app.apk \
    --test-devices="model=MediumPhone.arm,version=34,locale=en,orientation=portrait" \
    --test-case-ids="case1,case2"

Copy link
Contributor

@lfkellogg lfkellogg left a comment

Choose a reason for hiding this comment

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

Looks great!

`appdistribution:distribute` to launch AI Testing Agent.
}
}
const releaseTests = await Promise.all(releaseTestPromises);
utils.logSuccess(`${releaseTests.length} release test(s) started successfully`);
Copy link

Choose a reason for hiding this comment

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

"[all] started successfully" -- what if some could not be started due to out-of-quota or other reasons?
There doesn't seem to be any handling of that; this phrasing might be deceptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the perspective of the App Distribution API, the "release test" has been started as soon as createReleaseTest() returns successfully.

If the test matrix that's used "under the hood" fails for some reason (incl. out-of-quota), this will be caught below (line 279) - unless --test-non-blocking is specified. In that case the customer has to check the Firebase Console.

I understand that the TestLab API distinguishes the setup phase from running the actual test, but App Distribution doesn't.

if (!options.testNonBlocking) {
await awaitTestResults(releaseTest.name!, requests);
const releaseTestNames = new Set(releaseTests.map((rt) => rt.name!!));
Copy link

Choose a reason for hiding this comment

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

Type assertions are usually considered bad form. Prefer type narrowing, nullish coalescing ?? or ||...
This one tries to double assert (which doesn't really do anything extra)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to switching this to ?? "" or something similar

}
}
});

async function awaitTestResults(
releaseTestName: string,
releaseTestNames: Set<string>,
Copy link

Choose a reason for hiding this comment

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

Nit: I dislike passing by reference and modifying the original object - it would get messy if caller decides to reuse the object for anything else.
As a bonus, if you make a local copy, you could separate 1 vs many test cases & adjust the messages to use that.

requests: AppDistributionClient,
): Promise<void> {
for (let i = 0; i < TEST_MAX_POLLING_RETRIES; i++) {
utils.logBullet("the automated tests results are pending");
utils.logBullet("the automated test results are pending");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not new code, but log lines should generally be capitalized

Suggested change
utils.logBullet("the automated test results are pending");
utils.logBullet("The automated test results are pending...");

if (releaseTest.deviceExecutions.every((e) => e.state === "PASSED")) {
releaseTestNames.delete(releaseTestName);
if (releaseTestNames.size === 0) {
utils.logSuccess("automated test(s) passed!");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
utils.logSuccess("automated test(s) passed!");
utils.logSuccess("Automated test(s) passed!");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants