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

Update retry to improve machine placement on read and introduce internal/juju unit tests with mocking #433

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

hmlanigan
Copy link
Member

Description

Resolves TODO from #359 where minimal application import test was failing by improving retry on ReadApplication to wait for machines to exists when required. Does not apply to CAAS models, not subordinate applications.

Adds unit tests for internal/juju utilizing the stretchr test package. Currently there is coverage for ReadApplicationWithRetryOnNotFound. More can be added as we go. A new make target has been added to run them.

This involved refactoring parts of internal/juju to allow for mocks to be used by introducing new local interfaces and wrappers on the api clients to talk to juju.

Stretchr has some notable differences with the way tests are written in github.com/juju/juju. Notably: suite.Required() is equivalent to Assert() and suite.Assert() is equivalent to Check().

Type of change

  • Change in tests (one or several tests have been changed)
  • Bug fix (non-breaking change which fixes an issue)

QA steps

terraform {
  required_providers {
    juju = {
      version = ">= 0.9.0"
      source  = "juju/juju"
    }
  }
}

provider "juju" {
}

resource "juju_model" "testmodel" {
  name = "testme"
}

resource "juju_application" "application_three" {
  model = juju_model.testmodel.name
  units = 2
  charm {
    name = "juju-qa-test"
  }
}

resource "juju_application" "application_one" {
  model = juju_model.testmodel.name
  units = 0
  charm {
    name = "ntp"
  }
}

resource "juju_integration" "juju-info" {
  model = juju_model.testmodel.name
  application {
    name = juju_application.application_three.name
    endpoint = "juju-info"
  }
  application {
    name = juju_application.application_one.name
    endpoint = "juju-info"
  }
}

Additional notes

JUJU-5427

It's a known issue what not all machines may be noted in placement when
 an application is created, juju#376. This change will fix the test failures
 surfaced with an update to the framework v 1.5 and this bug.

However the bug still exists due to ordering of the Placement vales in the
string.

A hole exists for subordinate applications as noted in the code.
The data is include in ApplicationInfo results.
Setup to all for local units tests using mocks, but adding interfaces
and wrapping the juju api clients.
@hmlanigan hmlanigan requested a review from cderici March 14, 2024 20:41
@hmlanigan hmlanigan force-pushed the fix-machine-placement-on-read branch 2 times, most recently from cce2550 to 18dc1bd Compare March 14, 2024 20:54
@hmlanigan hmlanigan requested review from anvial and Aflynn50 and removed request for anvial March 14, 2024 20:59
Copy link
Contributor

@Aflynn50 Aflynn50 left a comment

Choose a reason for hiding this comment

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

Good amount of testing!

return nil
}
if output.Placement == "" {
return fmt.Errorf("ReadApplicationWithRetryOnNotFound: no machines found in output")
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message could be a bit confusing to a user, does "output" mean something to them?

Copy link
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

This is an amazing start for the unit tests! Code looks good, QA went well 👍

@hmlanigan hmlanigan added this to the 0.11.0 milestone Mar 15, 2024
The first unit tests using gomock and stretchr. Testing the Retry logic
of ReadApplicationWithRetryOnNotFound.

Return ApplicationMinimal Import test
@hmlanigan hmlanigan force-pushed the fix-machine-placement-on-read branch from c546a3a to 0e9402f Compare March 15, 2024 13:29
@hmlanigan
Copy link
Member Author

Fixed a test and squashed in the lint change with the major commit - required a force.

@hmlanigan hmlanigan merged commit a8b9c58 into juju:main Mar 15, 2024
22 of 25 checks passed
@hmlanigan hmlanigan deleted the fix-machine-placement-on-read branch March 15, 2024 14:11
@cderici cderici mentioned this pull request Mar 18, 2024
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