Skip to content

refactor: change the group command to add unit tests#1818

Merged
kian99 merged 3 commits intocanonical:v3from
kian99:group-test-cmd-refactor
Jan 21, 2026
Merged

refactor: change the group command to add unit tests#1818
kian99 merged 3 commits intocanonical:v3from
kian99:group-test-cmd-refactor

Conversation

@kian99
Copy link
Contributor

@kian99 kian99 commented Jan 20, 2026

Description

This PR refactors the group-related commands to add unit tests. I also found a bug in the remove-group command where the force flag had an incorrect name.

Fixes JUJU-9005

Engineering checklist

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

@kian99 kian99 requested a review from a team as a code owner January 20, 2026 10:11
@kian99 kian99 force-pushed the group-test-cmd-refactor branch 2 times, most recently from ff02d6f to c9ef512 Compare January 20, 2026 10:19
Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

lgtm bar the newclient stuff and fmt.Errorf consistency.

@kian99 kian99 force-pushed the group-test-cmd-refactor branch from c9ef512 to f48c35f Compare January 21, 2026 07:52
Comment on lines 49 to 51
jimmAPIFunc: func(store jujuclient.ClientStore, dialOpts *api.DialOpts) (JIMMAPI, error) {
return cmdMocks.client, nil
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a heads up on this change @ale8k, I need to pass in a store here since some of the other commands that are still integration testing, are re-using the addGroup command. Without being able to mock the store I can't run the command in an integration test.

@kian99 kian99 force-pushed the group-test-cmd-refactor branch from f48c35f to 51b7cfa Compare January 21, 2026 08:27
kian99 and others added 2 commits January 21, 2026 17:25
There was an attempt to simplify the client logic and create a `NewClient` method that client methods can use but the method `NewAPIRootWithDialOpts` needs is a little involved and requires initialisation and would panic unless initialised. Reverting the change until we find a better approach.
@kian99 kian99 merged commit 05e1eec into canonical:v3 Jan 21, 2026
9 of 10 checks 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.

3 participants