Skip to content

Tests for app-* commands #8909

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
68 changes: 68 additions & 0 deletions src/commands/apps-android-sha-create.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { expect } from "chai";
import * as sinon from "sinon";
import { Command } from "../command";
import * as projectUtils from "../projectUtils";
import * as apps from "../management/apps";
import { ShaCertificateType } from "../management/apps";
import * as utils from "../utils";
import { command, getCertHashType } from "./apps-android-sha-create";

describe("apps:android:sha:create", () => {
let sandbox: sinon.SinonSandbox;
let needProjectIdStub: sinon.SinonStub;
let createAppAndroidShaStub: sinon.SinonStub;
let promiseWithSpinnerStub: sinon.SinonStub;

beforeEach(() => {
sandbox = sinon.createSandbox();
needProjectIdStub = sandbox.stub(projectUtils, "needProjectId").returns("test-project-id");
createAppAndroidShaStub = sandbox
.stub(apps, "createAppAndroidSha")
.resolves({ name: "test-sha", shaHash: "test-hash", certType: ShaCertificateType.SHA_1 });
promiseWithSpinnerStub = sandbox.stub(utils, "promiseWithSpinner").callThrough();
});

afterEach(() => {
sandbox.restore();
});

it("should be a Command", () => {
expect(command).to.be.an.instanceOf(Command);
});

describe("action", () => {
it("should create a SHA certificate", async () => {
const shaHash = "A1:B2:C3:D4:E5:F6:A1:B2:C3:D4:E5:F6:A1:B2:C3:D4:E5:F6:A1:B2"; // SHA-1
await command.runner()("test-app-id", shaHash, {});

expect(needProjectIdStub).to.have.been.calledOnce;
expect(createAppAndroidShaStub).to.have.been.calledOnce;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test checks that createAppAndroidShaStub is called, but it could be more robust by also verifying that it's called with the correct arguments. This ensures the command is correctly interpreting its inputs and passing them to the management API.

      expect(createAppAndroidShaStub).to.have.been.calledOnceWith("test-project-id", "test-app-id", {
        shaHash: shaHash,
        certType: ShaCertificateType.SHA_1
      });

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we address this ^?

const spinnerText = promiseWithSpinnerStub.getCall(0).args[1];

Check warning on line 40 in src/commands/apps-android-sha-create.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
expect(spinnerText).to.include("Creating Android SHA certificate");
});
});

describe("getCertHashType", () => {
it("should return SHA_1 for a 40-character hash", () => {
const shaHash = "A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4E5F6A1B2";
expect(getCertHashType(shaHash)).to.equal(ShaCertificateType.SHA_1);
});

it("should return SHA_256 for a 64-character hash", () => {
const shaHash = "A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4E5F6A1B2";
expect(getCertHashType(shaHash)).to.equal(ShaCertificateType.SHA_256);
});

it("should return UNSPECIFIED for other hash lengths", () => {
const shaHash = "A1B2C3D4E5F6";
expect(getCertHashType(shaHash)).to.equal(
ShaCertificateType.SHA_CERTIFICATE_TYPE_UNSPECIFIED,
);
});

it("should handle colons in the hash", () => {
const shaHash = "A1:B2:C3:D4:E5:F6:A1:B2:C3:D4:E5:F6:A1:B2:C3:D4:E5:F6:A1:B2";
expect(getCertHashType(shaHash)).to.equal(ShaCertificateType.SHA_1);
});
});
});
2 changes: 1 addition & 1 deletion src/commands/apps-android-sha-create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { requireAuth } from "../requireAuth";
import { promiseWithSpinner } from "../utils";

function getCertHashType(shaHash: string): string {
export function getCertHashType(shaHash: string): string {

Check warning on line 9 in src/commands/apps-android-sha-create.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Missing JSDoc comment
shaHash = shaHash.replace(/:/g, "");
const shaHashCount = shaHash.length;
if (shaHashCount === 40) return ShaCertificateType.SHA_1.toString();
Expand All @@ -18,8 +18,8 @@
.description("add a SHA certificate hash for a given app id")
.before(requireAuth)
.action(
async (appId: string = "", shaHash: string = "", options: any): Promise<AppAndroidShaData> => {

Check warning on line 21 in src/commands/apps-android-sha-create.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type

Check warning on line 21 in src/commands/apps-android-sha-create.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Type string trivially inferred from a string literal, remove type annotation

Check warning on line 21 in src/commands/apps-android-sha-create.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Type string trivially inferred from a string literal, remove type annotation
const projectId = needProjectId(options);

Check warning on line 22 in src/commands/apps-android-sha-create.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe argument of type `any` assigned to a parameter of type `{ projectId?: string | undefined; project?: string | undefined; rc?: RC | undefined; }`

const shaCertificate = await promiseWithSpinner<AppAndroidShaData>(
async () =>
Expand All @@ -28,7 +28,7 @@
certType: getCertHashType(shaHash),
}),
`Creating Android SHA certificate ${clc.bold(
options.shaHash,

Check warning on line 31 in src/commands/apps-android-sha-create.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .shaHash on an `any` value

Check warning on line 31 in src/commands/apps-android-sha-create.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe argument of type `any` assigned to a parameter of type `string | number`
)}with Android app Id ${clc.bold(appId)}`,
);

Expand Down
40 changes: 40 additions & 0 deletions src/commands/apps-android-sha-delete.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { expect } from "chai";
import * as sinon from "sinon";
import { Command } from "../command";
import * as projectUtils from "../projectUtils";
import * as apps from "../management/apps";
import * as utils from "../utils";
import { command } from "./apps-android-sha-delete";

describe("apps:android:sha:delete", () => {
let sandbox: sinon.SinonSandbox;
let needProjectIdStub: sinon.SinonStub;
let deleteAppAndroidShaStub: sinon.SinonStub;
let promiseWithSpinnerStub: sinon.SinonStub;

beforeEach(() => {
sandbox = sinon.createSandbox();
needProjectIdStub = sandbox.stub(projectUtils, "needProjectId").returns("test-project-id");
deleteAppAndroidShaStub = sandbox.stub(apps, "deleteAppAndroidSha").resolves();
promiseWithSpinnerStub = sandbox.stub(utils, "promiseWithSpinner").callThrough();
});

afterEach(() => {
sandbox.restore();
});

it("should be a Command", () => {
expect(command).to.be.an.instanceOf(Command);
});

describe("action", () => {
it("should delete a SHA certificate", async () => {
await command.runner()("test-app-id", "test-sha-id", {});

expect(needProjectIdStub).to.have.been.calledOnce;
expect(deleteAppAndroidShaStub).to.have.been.calledOnce;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

To make this test more specific, consider asserting that deleteAppAndroidShaStub is called with the expected arguments (projectId, appId, and shaId). This will ensure the command is correctly parsing and passing its arguments.

Suggested change
expect(deleteAppAndroidShaStub).to.have.been.calledOnce;
expect(deleteAppAndroidShaStub).to.have.been.calledOnceWith("test-project-id", "test-app-id", "test-sha-id");

Copy link
Contributor

Choose a reason for hiding this comment

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

^ can we address this as well?

const spinnerText = promiseWithSpinnerStub.getCall(0).args[1];

Check warning on line 36 in src/commands/apps-android-sha-delete.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
expect(spinnerText).to.include("Deleting Android SHA certificate hash");
});
});
});
106 changes: 106 additions & 0 deletions src/commands/apps-android-sha-list.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import { expect } from "chai";
import * as sinon from "sinon";
import * as Table from "cli-table3";
import { Command } from "../command";
import * as projectUtils from "../projectUtils";
import * as apps from "../management/apps";
import { AppAndroidShaData, ShaCertificateType } from "../management/apps";
import * as utils from "../utils";
import { command, logCertificatesList, logCertificatesCount } from "./apps-android-sha-list";

describe("apps:android:sha:list", () => {
let sandbox: sinon.SinonSandbox;
let promiseWithSpinnerStub: sinon.SinonStub;

beforeEach(() => {
sandbox = sinon.createSandbox();
sandbox.stub(projectUtils, "needProjectId").returns("test-project-id");
sandbox.stub(apps, "listAppAndroidSha");
promiseWithSpinnerStub = sandbox.stub(utils, "promiseWithSpinner");
});

afterEach(() => {
sandbox.restore();
});

it("should be a Command", () => {
expect(command).to.be.an.instanceOf(Command);
});

describe("action", () => {
it("should list SHA certificates", async () => {
const certificates: AppAndroidShaData[] = [
{
name: "projects/p/androidApps/a/sha/s1",
shaHash: "h1",
certType: ShaCertificateType.SHA_1,
},
{
name: "projects/p/androidApps/a/sha/s2",
shaHash: "h2",
certType: ShaCertificateType.SHA_256,
},
];
promiseWithSpinnerStub.resolves(certificates);

await command.runner()("test-app-id", {});

Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test stubs promiseWithSpinner directly, which prevents the actual logic inside the action (the call to listAppAndroidSha) from being executed. A more effective test would be to let promiseWithSpinner execute (using callThrough) and stub listAppAndroidSha instead. This would allow you to verify that listAppAndroidSha is called with the correct arguments, which is the primary responsibility of this command's action.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this!

expect(promiseWithSpinnerStub).to.have.been.calledOnce;
const spinnerText = promiseWithSpinnerStub.getCall(0).args[1];

Check warning on line 49 in src/commands/apps-android-sha-list.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value
expect(spinnerText).to.include("Preparing the list");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also check that the logger logged the list that you stubbed above?

});

it('should display "No SHA certificate hashes found." if no certificates exist', async () => {
promiseWithSpinnerStub.resolves([]);

await command.runner()("test-app-id", {});

// No assertion needed here, we are just checking that it does not throw.
});
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test only checks that the command doesn't throw an error when no certificates are found. A stronger assertion would be to verify that the correct message is logged to the user. You can achieve this by spying on logger.info and checking that it's called with a message including "No SHA certificate hashes found.".

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we address this?

});

describe("logCertificatesList", () => {
it("should print a table of certificates", () => {
const certificates: AppAndroidShaData[] = [
{
name: "projects/p/androidApps/app1/sha/sha1",
shaHash: "hash1",
certType: ShaCertificateType.SHA_1,
},
{
name: "projects/p/androidApps/app2/sha/sha2",
shaHash: "hash2",
certType: ShaCertificateType.SHA_256,
},
];
const tableSpy = sandbox.spy(Table.prototype, "push");

logCertificatesList(certificates);

expect(tableSpy.getCall(0).args[0]).to.deep.equal([
"app1",
"sha1",
"hash1",
ShaCertificateType.SHA_1,
]);
expect(tableSpy.getCall(1).args[0]).to.deep.equal([
"app2",
"sha2",
"hash2",
ShaCertificateType.SHA_256,
]);
});
});

describe("logCertificatesCount", () => {
it("should print the total number of certificates", () => {
logCertificatesCount(5);
// No assertion needed here, we are just checking that it does not throw.
});

it("should not print if count is 0", () => {
logCertificatesCount(0);
// No assertion needed here, we are just checking that it does not throw.
});
});
});
4 changes: 2 additions & 2 deletions src/commands/apps-android-sha-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { requireAuth } from "../requireAuth";
import { logger } from "../logger";
import { promiseWithSpinner } from "../utils";

function logCertificatesList(certificates: AppAndroidShaData[]): void {
export function logCertificatesList(certificates: AppAndroidShaData[]): void {
if (certificates.length === 0) {
logger.info("No SHA certificate hashes found.");
return;
Expand All @@ -27,7 +27,7 @@ function logCertificatesList(certificates: AppAndroidShaData[]): void {
logger.info(table.toString());
}

function logCertificatesCount(count: number = 0): void {
export function logCertificatesCount(count: number = 0): void {
if (count === 0) {
return;
}
Expand Down
146 changes: 146 additions & 0 deletions src/commands/apps-create.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
import { expect } from "chai";
import * as sinon from "sinon";
import { Command } from "../command";
import * as projectUtils from "../projectUtils";
import { FirebaseError } from "../error";
import * as apps from "../management/apps";
import { AppPlatform } from "../management/apps";
import * as prompt from "../prompt";
import { command, logPostAppCreationInformation } from "./apps-create";

describe("apps:create", () => {
let sandbox: sinon.SinonSandbox;
let needProjectIdStub: sinon.SinonStub;
let getAppPlatformStub: sinon.SinonStub;
let sdkInitStub: sinon.SinonStub;
let selectStub: sinon.SinonStub;

beforeEach(() => {
sandbox = sinon.createSandbox();
needProjectIdStub = sandbox.stub(projectUtils, "needProjectId").returns("test-project-id");
getAppPlatformStub = sandbox.stub(apps, "getAppPlatform");
sdkInitStub = sandbox.stub(apps, "sdkInit").resolves({
name: "test-name",
projectId: "test-project-id",
appId: "test-app-id",
platform: AppPlatform.WEB,
displayName: "test-display-name",
});
selectStub = sandbox.stub(prompt, "select");
});

afterEach(() => {
sandbox.restore();
});

it("should be a Command", () => {
expect(command).to.be.an.instanceOf(Command);
});

describe("action", () => {
it("should throw if platform is not provided in non-interactive mode", async () => {
getAppPlatformStub.returns(AppPlatform.ANY);
const options = { nonInteractive: true };
await expect(command.runner()("", undefined, options)).to.be.rejectedWith(
FirebaseError,
"App platform must be provided",
);
expect(needProjectIdStub).to.have.been.calledOnce;
});

it("should prompt for platform if not provided in interactive mode", async () => {
getAppPlatformStub.withArgs("").returns(AppPlatform.ANY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be here?

getAppPlatformStub.withArgs("IOS").returns(AppPlatform.IOS);
selectStub.resolves("IOS");
const options = { nonInteractive: false };
await command.runner()("", "test-display-name", options);
expect(selectStub).to.have.been.calledOnce;
expect(sdkInitStub).to.have.been.calledOnceWith(
AppPlatform.IOS,
sinon.match({
displayName: "test-display-name",
nonInteractive: false,
}),
);
});

it("should create an iOS app", async () => {
getAppPlatformStub.returns(AppPlatform.IOS);
const options = { bundleId: "test-bundle-id" };
await command.runner()("IOS", "test-display-name", options);
expect(sdkInitStub).to.have.been.calledOnceWith(
AppPlatform.IOS,
sinon.match({
bundleId: "test-bundle-id",
displayName: "test-display-name",
}),
);
});

it("should create an Android app", async () => {
getAppPlatformStub.returns(AppPlatform.ANDROID);
const options = { packageName: "test-package-name" };
await command.runner()("ANDROID", "test-display-name", options);
expect(sdkInitStub).to.have.been.calledOnceWith(
AppPlatform.ANDROID,
sinon.match({
packageName: "test-package-name",
displayName: "test-display-name",
}),
);
});

it("should create a Web app", async () => {
getAppPlatformStub.returns(AppPlatform.WEB);
const options = {};
await command.runner()("WEB", "test-display-name", options);
expect(sdkInitStub).to.have.been.calledOnceWith(
AppPlatform.WEB,
sinon.match({
displayName: "test-display-name",
}),
);
});
});

describe("logPostAppCreationInformation", () => {
it("should log basic app information", () => {
const appMetadata: apps.WebAppMetadata = {
name: "test-name",
projectId: "test-project-id",
appId: "test-app-id",
platform: AppPlatform.WEB,
displayName: "test-display-name",
};
logPostAppCreationInformation(appMetadata, AppPlatform.WEB);
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The tests in this describe block only check that the logging function doesn't throw an error, which is a very weak assertion. To make them more valuable, you should assert that the correct information is being logged.

// No assertion needed here, we are just checking that it does not throw.
});

it("should log iOS specific information", () => {
const appMetadata: apps.IosAppMetadata = {
name: "test-name",
projectId: "test-project-id",
appId: "test-app-id",
platform: AppPlatform.IOS,
displayName: "test-display-name",
bundleId: "test-bundle-id",
appStoreId: "test-app-store-id",
};
logPostAppCreationInformation(appMetadata, AppPlatform.IOS);
// No assertion needed here, we are just checking that it does not throw.
});

it("should log Android specific information", () => {
const appMetadata: apps.AndroidAppMetadata = {
name: "test-name",
projectId: "test-project-id",
appId: "test-app-id",
platform: AppPlatform.ANDROID,
displayName: "test-display-name",
packageName: "test-package-name",
};
logPostAppCreationInformation(appMetadata, AppPlatform.ANDROID);
// No assertion needed here, we are just checking that it does not throw.
});
});
});
2 changes: 1 addition & 1 deletion src/commands/apps-create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { logger } from "../logger";
import { Options } from "../options";
import { select } from "../prompt";

function logPostAppCreationInformation(
export function logPostAppCreationInformation(
appMetadata: IosAppMetadata | AndroidAppMetadata | WebAppMetadata,
appPlatform: AppPlatform,
): void {
Expand Down
Loading
Loading