Skip to content

test: use new integration test suite for dial tests#1879

Merged
kian99 merged 4 commits intocanonical:v3from
kian99:refactor-dial-tests
Feb 23, 2026
Merged

test: use new integration test suite for dial tests#1879
kian99 merged 4 commits intocanonical:v3from
kian99:refactor-dial-tests

Conversation

@kian99
Copy link
Contributor

@kian99 kian99 commented Feb 20, 2026

Description

This PR updates the dial tests in the jujuclient to use the new test suite that uses real Juju controllers and removes the final usages of Juju's ConnSuite (but doesn't remove the ConnSuite itself just yet).

I've also removed the jujuclient package's test files cloud_test.go and storage_test.go which are being removed in #1876 and another pending PR for switching clouds over to the Juju API client.

It's worth noting that tests like TestDial are taking > 5s because the suite creates 3 models and then tears them down at the end. In a follow-up PR I'll look at making this model setup optional.

Engineering checklist

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

@kian99 kian99 requested a review from a team as a code owner February 20, 2026 08:58
"github.com/canonical/jimm/v3/internal/testutils/jimmtest"
)

type jujuclientSuite struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

yeaaaaah

@kian99 kian99 force-pushed the refactor-dial-tests branch from 2fb181c to e70a123 Compare February 20, 2026 13:43
@kian99
Copy link
Contributor Author

kian99 commented Feb 20, 2026

@SimoneDutto One tricky thing with this PR, I've had to move the dial_test.go file from jujuclient to testing because the CI is setup to only run the tests in testing as integration tests with real controllers. Maybe in the future we can simplify and just run all the tests in one job, the unit tests should be pretty fast in any case compared to the integration tests so the difference will hopefully be negligible and then you can put tests anywhere as needed.

@SimoneDutto
Copy link
Contributor

@SimoneDutto One tricky thing with this PR, I've had to move the dial_test.go file from jujuclient to testing because the CI is setup to only run the tests in testing as integration tests with real controllers. Maybe in the future we can simplify and just run all the tests in one job, the unit tests should be pretty fast in any case compared to the integration tests so the difference will hopefully be negligible and then you can put tests anywhere as needed.

yeah, we can try. At the time of setting up these things unit tests were still half e2e and half unit.

@@ -0,0 +1,115 @@
// Copyright 2025 Canonical.
Copy link
Collaborator

Choose a reason for hiding this comment

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

2026?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was about to say "Lol good one" until I realised this was a legitimate comment. I just moved the file and made some tweaks but I guess git is seeing that as too many changes to count as a rename - see internal/jujuclient/dial_test.go which was removed in this PR.


var _ = gc.Suite(&dialSuite{})

func (s *dialSuite) TestDial(c *gc.C) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could take it a step further and get rid of gc in favor of qt.. but..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently working on that 👍

@kian99 kian99 force-pushed the refactor-dial-tests branch from 5bb9f38 to d522b22 Compare February 23, 2026 09:55
@kian99 kian99 merged commit fc38161 into canonical:v3 Feb 23, 2026
8 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