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

refactor: use later begin rather than re-instantiating the harness charm #147

Conversation

tonyandrewmeyer
Copy link
Contributor

Description

The charm conditionally sets the unit status (and does some other work, including observing) based on the result of setting up a configuration object during __init__. This isn't a practice that Charm Tech recommends, and it's one that currently works in production (if defer is not used) but is not supported by Harness (which won't be changed, unfortunately).

At the moment, the tests have a workaround where the harness charm is reinstantiated using private methods of both the harness framework and harness itself. This PR removes the reinstantiation, so that the private use is not required.

The actual change is pretty straightforward: it just waits until all the setup is complete (in particular, setting the config values) before calling harness.begin() (which executes the charm's __init__).

This PR does not change any of the tests, other than replacing the reinstantiate_charm() call with self.harness.begin(). To minimise the impact of the change, these tests are moved to a separate TestCase subclass. An alternative that would result in fewer lines changing (e.g. in blame) would be to remove the begin() from the existing class and add it in explicitly to each test case (that doesn't do the reinstantiate()), but this change seemed cleaner.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation - N/A
  • I have added tests that validate the behaviour of the software - N/A
  • I validated that new and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules - N/A
  • I have bumped the version of the library - N/A

@tonyandrewmeyer tonyandrewmeyer requested a review from a team as a code owner April 3, 2024 00:47
@ghislainbourgeois
Copy link
Contributor

Thanks for the PR and the detailed explanation!

@ghislainbourgeois ghislainbourgeois merged commit a2cde23 into canonical:main Apr 3, 2024
14 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the remove-charm-reinit-in-unit-tests branch April 8, 2024 06:41
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.

2 participants