Added a very basic smoke test. Run it with `tox -e smoke` #800

pengale · 2022-07-14T21:54:18Z

This PR adds a smoke test will build a new "smoke" charm in test/charms/test_smoke.
The test will attempt to deploy xenial, bionic and focal versions of the charm.

Checklist

[N] Have any types changed? If so, have the type annotations been updated?
[N/A] If this code exposes model data, does it do so thoughtfully, in a way that aids understanding?
```
We want to avoid simple passthrough of model data, without contextualization, where possible.
```
[Y] Do error messages, if any, present charm authors or operators with enough information to solve the problem?

QA steps

Make sure that you are on a box with charmcraft and juju installed, and that you are connected to a "machine" controller, such as a local lxd cloud. Then run:

tox -e smoke

Documentation changes

This adds a step to the release process. Right now, it's a manual step: run tox -e smoke before releasing a version of ops.

Bug reference

This would have caught Operator Framework Not Working on Xenial #798

Changelog

Adds smoke test charm to test/charms dir.
Adds tox smoke env that builds and deploys the charm.

Notes

There are a few wonky things about this, including how we inject the current ops package, and the fact that we test with just the focal version of the charm. Future versions of this smoke test should clear those wonky bits up, as well as add automation.

This requires a development box with an active juju controller, and charmcraft installed. The smoke test will build the "smoke" charm in test/charms/test_smoke and try to deploy xenial, bionic and focal versions of the charm. There are a few wonky things about this, including how we inject the current ops package, and the fact that we test with just the focal version of the charm. Future versions of this smoke test should clear those wonky bits up, as well as add automation.

rwcarlsen · 2022-07-20T16:45:00Z

Would it be worth it to have tox -e smoke set up the entire environment inside lxd containers rather than doing stuff directly with juju on my/the host machine?

pengale · 2022-07-21T12:20:59Z

Would it be worth it to have tox -e smoke set up the entire environment inside lxd containers rather than doing stuff directly with juju on my/the host machine?

Eventually, yes!

... though we also want this to work on AWS and other substrates, so you'd need to make it configurable.

But that's out of scope for this PR, which was just "get this thing setup while I'm letting my brain work on dashboard on a background thread."

pengale · 2022-07-21T12:22:04Z

Also, thank you for taking a look :-)

And one more thing: this doesn't work on AWS, because pytest-operator can't handle the self signed cert. I think that's a bug in pytest-operator and/or python-libjuju. I don't think that we need to handle it in this code.

rwcarlsen

Looks fine. Just one small note below about adding some setup explanation in a couple spots.

rwcarlsen · 2022-07-27T18:22:35Z

test/charms/test_smoke/README.md

+## Usage
+
+Run `tox -e smoke` in the root directory of this repository to build and deploy this charm.


Could you include the QA steps info from the PR description here - the stuff about how to make sure your environment is prepared to run the tests. Also probably worth noting that same info as a comment inside tox.ini.

PietroPasotti

Looks good! Just a few minor comments

PietroPasotti · 2022-08-03T09:17:52Z

test/smoke/test_smoke.py

+    # older versions of Juju.)
+    charm = await ops_test.build_charm("./test/charms/test_smoke/")
+
+    for series in ['focal', 'bionic', 'xenial']:


use pytest.mark.parametrize to give a better error message (use ids=['focal', ...] for bonus points)

I started in on this, and it ended up adding a lot of complexity. The build needs to get moved into a separate routine, and then we need a data structure to track our charm artifact. None of that is difficult to write, but it didn't feel worthwhile to do so until this test gets more complex.

PietroPasotti · 2022-08-03T09:18:21Z

test/smoke/test_smoke.py

+logger = logging.getLogger(__name__)
+
+
+CURRENT_CHANNEL = '20.04'  # Focal


where is this used?

PietroPasotti · 2022-08-03T09:19:52Z

test/charms/test_smoke/config.yaml

+#
+# Learn more about config at: https://juju.is/docs/sdk/config
+
+options: {}


IIRC we can remove the file completely if there's no config

PietroPasotti · 2022-08-03T09:21:07Z

test/charms/test_smoke/config.yaml

@@ -0,0 +1,10 @@
+# Copyright 2022 Penelope Valentine Gale


Should the copyright not be Canonical?

rwcarlsen · 2022-08-10T20:38:45Z

test/charms/test_smoke/metadata.yaml

+# Copyright 2022 Penelope Valentine Gale
+# See LICENSE file for licensing details.
+


Looks like maybe fixing this copyright line got missed before the merge?

benhoyt · 2022-08-14T23:07:01Z

Is this being run by GitHub Actions? If so, I'm wondering why the tests are succeeding when the Xenial fix #810 isn't yet merged?

pengale · 2022-08-15T14:00:24Z

Is this being run by GitHub Actions? If so, I'm wondering why the tests are succeeding when the Xenial fix #810 isn't yet merged?

The smoke tests are manual tests for now -- this is something that I timeboxed and snuck in as a bonus.

We intend to automate them in the future. (Probably the next time I need to take a thinking break on what I'm officially working on for a bit.)

) This requires a development box with an active juju controller, and charmcraft installed. The smoke test will build the "smoke" charm in test/charms/test_smoke and try to deploy xenial, bionic and focal versions of the charm. There are a few wonky things about this, including how we inject the current ops package, and the fact that we test with just the focal version of the charm. Future versions of this smoke test should clear those wonky bits up, as well as add automation.

pengale force-pushed the add-smoke-test branch from ff212a2 to 614f6c6 Compare July 14, 2022 21:55

Fixed up linter errors, and excluded smoke from unit tests.

10b1e2e

pengale assigned rwcarlsen and PietroPasotti Jul 19, 2022

Merge branch 'main' into add-smoke-test

e555ab1

rwcarlsen approved these changes Jul 27, 2022

View reviewed changes

PietroPasotti approved these changes Aug 3, 2022

View reviewed changes

PietroPasotti mentioned this pull request Aug 5, 2022

Fix model types #810

Merged

1 task

Addressed PR comments.

2862904

rwcarlsen approved these changes Aug 10, 2022

View reviewed changes

pengale added 2 commits August 10, 2022 19:34

Dropped config file -- not necessary.

0e28783

Merge branch 'main' into add-smoke-test

c944bc2

pengale merged commit c2ffe25 into canonical:main Aug 10, 2022

rwcarlsen reviewed Aug 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a very basic smoke test. Run it with `tox -e smoke` #800

Added a very basic smoke test. Run it with `tox -e smoke` #800

pengale commented Jul 14, 2022 •

edited by rwcarlsen

rwcarlsen commented Jul 20, 2022

pengale commented Jul 21, 2022

pengale commented Jul 21, 2022

rwcarlsen left a comment

rwcarlsen Jul 27, 2022

PietroPasotti left a comment

PietroPasotti Aug 3, 2022

pengale Aug 10, 2022

PietroPasotti Aug 3, 2022

PietroPasotti Aug 3, 2022

PietroPasotti Aug 3, 2022

rwcarlsen Aug 10, 2022

benhoyt commented Aug 14, 2022

pengale commented Aug 15, 2022 •

edited

		## Usage

		Run `tox -e smoke` in the root directory of this repository to build and deploy this charm.

		logger = logging.getLogger(__name__)


		CURRENT_CHANNEL = '20.04' # Focal

		# Copyright 2022 Penelope Valentine Gale
		# See LICENSE file for licensing details.

Added a very basic smoke test. Run it with tox -e smoke #800

Added a very basic smoke test. Run it with tox -e smoke #800

Conversation

pengale commented Jul 14, 2022 • edited by rwcarlsen

Checklist

QA steps

Documentation changes

Bug reference

Changelog

Notes

rwcarlsen commented Jul 20, 2022

pengale commented Jul 21, 2022

pengale commented Jul 21, 2022

rwcarlsen left a comment

Choose a reason for hiding this comment

rwcarlsen Jul 27, 2022

Choose a reason for hiding this comment

PietroPasotti left a comment

Choose a reason for hiding this comment

PietroPasotti Aug 3, 2022

Choose a reason for hiding this comment

pengale Aug 10, 2022

Choose a reason for hiding this comment

PietroPasotti Aug 3, 2022

Choose a reason for hiding this comment

PietroPasotti Aug 3, 2022

Choose a reason for hiding this comment

PietroPasotti Aug 3, 2022

Choose a reason for hiding this comment

rwcarlsen Aug 10, 2022

Choose a reason for hiding this comment

benhoyt commented Aug 14, 2022

pengale commented Aug 15, 2022 • edited

Added a very basic smoke test. Run it with `tox -e smoke` #800

Added a very basic smoke test. Run it with `tox -e smoke` #800

pengale commented Jul 14, 2022 •

edited by rwcarlsen

pengale commented Aug 15, 2022 •

edited