Skip to content

Conversation

@charithaT07
Copy link
Collaborator

@charithaT07 charithaT07 commented Jan 16, 2026

Summary

Brief description of what this PR does.

This PR delivers a comprehensive unit test suite for the b2c-cli package, covering key CLI commands such as WebDAV, Docs, Job, Sites, MRT, Auth, Code, and ODS.

In addition to expanding test coverage, it standardizes and improves test patterns by introducing shared helpers for:

  • Isolating config and environment to ensure tests run hermetically without depending on local developer settings.
  • Providing a unified command creation utility to guarantee consistent initialization across tests.
  • Refactoring tests to remove redundant patterns and ensure compliance with Mocha and ESLint best practices.

To enable testing, minor, controlled changes were made to source files of some commands. These test seams allow stubbing internal behavior in isolation without affecting functionality, avoiding reliance on network, filesystem, or local environment.

The PR also consolidates test helper logic by removing ODS-specific helpers and aligning ODS tests with the new common testing approach, improving maintainability and readability across the test suite.

b2c-cli package coverage is now 85%

Testing

How was this tested?


  • Tests pass (pnpm test)
  • Code is formatted (pnpm run format)

@charithaT07 charithaT07 marked this pull request as ready for review January 16, 2026 14:34
@charithaT07 charithaT07 requested a review from clavery as a code owner January 16, 2026 14:34
@charithaT07 charithaT07 force-pushed the W-20893569-b2c-cli-unit-tests branch from b79fa18 to c703ca3 Compare January 16, 2026 16:16
@charithaT07 charithaT07 marked this pull request as draft January 16, 2026 16:19
Copy link
Collaborator

@clavery clavery left a comment

Choose a reason for hiding this comment

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

Few comments on the style of dependency injection for testing here. I'd like to keep the number of lines supporting tests in the commands to a minimum so if the patterns I mentioned work that would be a better approach I think

}),
};

protected async deleteCartridges(cartridges: Parameters<typeof deleteCartridges>[1]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems this might be to mock the underlying SDK with sinon. But it's adding quite a bit of lines and noise. Can we try simplifying the dependency injection pattern here. Maybe we can support the run method with default arguments set to the imported SDK methods? So the tests can pass in their mocks.

  async run(
    // we'll have to disable this eslint globally but that's ok
    // eslint-disable-next-line unicorn/no-object-as-default-parameter
    operations = {
      uploadCartridges,
      deleteCartridges,
      getActiveCodeVersion,
      reloadCodeVersion,
    },
  ): Promise<DeployResult> {
    const {uploadCartridges, deleteCartridges, getActiveCodeVersion, reloadCodeVersion} = operations;

That way only the run method needs to change and the rest of the code stays the same.

Or maybe using a protected member with the operations we want to mock and then mock this in the tests?

   protected operations = {
     uploadCartridges,
     deleteCartridges,
     getActiveCodeVersion,
     reloadCodeVersion,
   };

//

await operations.uploadCartridges

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, Thanks Charles! I’ll look at simplifying the dependency injection pattern for these commands and see how we can reduce the test scaffolding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems very similar to the config-isolation in the SDK. Any way we can share that helper code to keep it consistent?

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.

3 participants