test: port integration tests from pytest-operator to Jubilant #326

benhoyt · 2025-04-07T04:45:09Z

You didn't ask for it :-), but this charm has the privilege of being one of the first charms to get its integration tests ported from pytest-operator (and python-libjuju) to Jubilant. We chose this charm because it isn't trivial, but it also wasn't huge, and it has a decent number of semi-complex integration tests.

It's relatively straight-forward stuff, and simpler in most cases. The tests seem to run in a similar time, or a bit faster: 46 minutes on Jubilant vs 51 minutes on pytest-operator.

A few notes:

I implemented a few lightweight fixtures: juju to create a temporary Juju model, charm_file to find the charm file, and I've updated app_fixture to use Jubilant and return a lightweight types.App object. Also the abort_on_fail fixture which is part of pytest-operator.
All use of async and python-libjuju is gone

Let me know if you have any questions or want to discuss!

benhoyt · 2025-04-10T03:29:26Z

tests/integration/conftest.py



-@fixture(scope="module", name="app_config")
+@pytest.fixture(scope="session", name="app_config")


Many of these should really be session-scoped, as they don't change per test.

Our integration tests are running the integration module by module (to parallelize them), this doesn't have much impact

You're right, it shouldn't have any real impact -- it's just a better way to signal intent.

benhoyt · 2025-04-10T03:35:44Z

tests/integration/conftest.py

-    await model.wait_for_idle()
-    await saml_app.set_config(  # type: ignore[attr-defined]
+
+    def all_units_idle(status: jubilant.Status) -> bool:


I've opened canonical/jubilant#91 to track whether we should add a jubilant.all_units_idle helper to do this. I'm not convinced it's worth it, though -- there are likely better ways of checking what we want here looking at app/unit status instead.

benhoyt · 2025-04-10T03:39:28Z

tests/integration/test_charm.py

-    test_discourse_srv_status_ok()
-
-
-@pytest.mark.skip(reason="Frequent timeouts")


I've removed this test rather than porting it to Jubilant, given that it wasn't running anyway. But maybe you guys can port this one if you need it. :-)

I think we should port it too, otherwise it'll disappear from the code base, and we might forget about it

benhoyt · 2025-04-10T04:43:11Z

pyproject.toml

@@ -54,3 +54,6 @@ check_untyped_defs = true

 [tool.pylint.SIMILARITIES]
 min-similarity-lines=10
+disable = [
+	"W0621",  # Redefining name %r from outer scope (line %s)


This is rather annoying for conftest.py, when defining pytest fixtures, for example a fixture named juju and then using juju as a local variable in another fixture (which is common). So I've turned it off.

gregory-schiano

Stopped at test_discourse_srv_status_ok test, we resume on Monday

gregory-schiano · 2025-04-11T15:00:20Z

tests/integration/conftest.py



-@fixture(scope="module", name="app_config")
+@pytest.fixture(scope="session", name="app_config")


Our integration tests are running the integration module by module (to parallelize them), this doesn't have much impact

gregory-schiano · 2025-04-11T15:03:27Z

tests/integration/conftest.py

+def discourse_address_fixture(app: types.App, juju: jubilant.Juju):
+    """Get discourse web address."""
+    status = juju.status()
+    unit_ip = status.apps[app.name].units[app.name + "/0"].address


Sometimes juju status returns APIv6, is it handled by jubilant ?

Yes, this field is just a str, so both IPv6 and IPv6 will come through fine.

I wonder which address should be used:

# st = juju.status() In [32]: st.apps["minio"].address Out[32]: '10.152.183.203' In [33]: st.apps["minio"].units["minio/2"].address Out[33]: '10.1.0.89'

In my case, there's only one unit, minio/2.
Both addresses work for me.

Which also makes me wonder if we're guaranteed to get unit id 0, or if it's better to do something like list(status.apps[...].units.values())[0] ?

gregory-schiano · 2025-04-11T15:14:49Z

tests/integration/conftest.py

-        return action.results
+    keep_models = cast(bool, request.config.getoption("--keep-models"))
+    with jubilant.temp_model(keep=keep_models) as juju:
+        juju.wait_timeout = 10 * 60


What is it used for ?

Juju.wait_timeout is the default timeout for wait() (in seconds) if that method’s timeout parameter is not specified.

In Jubilant the default wait_timeout is 3 minutes. This changes it to 10 minutes, which is the same as the pytest-operator default wait_for_idle timeout, just to keep things consistent with that.

tests/integration/conftest.py

gregory-schiano · 2025-04-11T15:49:02Z

tests/integration/conftest.py

+
+
+@pytest.fixture(autouse=True)
+def abort_on_fail(request: pytest.FixtureRequest):


Is it necessary ? And should be play with pytest behavior from conftest ?

Yeah, it's kind of necessary for easier migration from pytest-operator's abort_on_fail. I don't actually love this either, but it's a way to get other tests in a module to fail if previous ones have failed.

gregory-schiano · 2025-04-11T15:52:53Z

tests/integration/test_charm.py

-    stdout = action.results.get("stdout")
-    stderr = action.results.get("stderr")
-    assert code == 0, f"{cmd} failed ({code}): {stderr or stdout}"
+    juju.exec(cmd, unit=app.name + "/0", wait=60)


We were previously logging the stdout or stderr which won't be the case now if I understood jubilant behavior well (in case of exec failure)

Previously we were logging stdout or stderr in the case of the failed assert code == 0. Jubilant's exec raises an exception automatically. However, you're right -- the error message will be less than helpful. I've opened canonical/jubilant#97 to track.

gregory-schiano · 2025-04-11T15:58:42Z

tests/integration/test_charm.py

        {
-            "s3_enabled": "true",
+            "s3_enabled": True,


There's some hidden side effect in action here:
https://github.com/canonical/jubilant/blob/695a500f7545f234acdc7088c9579b4c8659797b/jubilant/_juju.py#L870-L871

I'd rather not do any manipulation on the input

This is by design, and I believe it's actually an improvement. For boolean config types (which s3_enabled is), you provide a proper Python bool type, which Jubilant then formats as "true" or "false" which is what Juju expects. So it's "using proper Python types" rather than "manipulation of the input".

@gregory-schiano

Thanks @gregory-schiano: canonical/discourse-k8s-operator#326 (comment)

@gregory-schiano

Thanks @gregory-schiano: canonical/discourse-k8s-operator#326 (comment)

tests/integration/conftest.py

dimaqq · 2025-04-14T05:43:10Z

tests/integration/conftest.py

+def discourse_address_fixture(app: types.App, juju: jubilant.Juju):
+    """Get discourse web address."""
+    status = juju.status()
+    unit_ip = status.apps[app.name].units[app.name + "/0"].address


I wonder which address should be used:

# st = juju.status() In [32]: st.apps["minio"].address Out[32]: '10.152.183.203' In [33]: st.apps["minio"].units["minio/2"].address Out[33]: '10.1.0.89'

In my case, there's only one unit, minio/2.
Both addresses work for me.

Which also makes me wonder if we're guaranteed to get unit id 0, or if it's better to do something like list(status.apps[...].units.values())[0] ?

dimaqq · 2025-04-14T06:06:08Z

tests/integration/test_saml.py

@@ -38,8 +38,8 @@ async def test_saml_login(  # pylint: disable=too-many-locals,too-many-arguments
    password = "test-discourse-k8s-password"  # nosecue
    saml_helper.register_user(username=username, email=email, password=password)

-    action_result = await run_action(app.name, "create-user", email=email)
-    assert "user" in action_result
+    task = juju.run(app.name + "/0", "create-user", {"email": email})


Suggested change

task = juju.run(app.name + "/0", "create-user", {"email": email})

task = juju.run(app.name + "/0", "create-user", params={"email": email})

Here and in the other test file.
I could kindof guess the intent behind the third argument, but implicit is better than explicit, isn't it?

Re the unit vs app IP: sometimes the app IP address is different, so I'm sticking with unit address like the original tests here.

Re "/0", yes, unit IDs are guaranteed by Juju to be incrementing integers and start at "0", so this is common.

Re params=, in Jubilant I haven't made this a keyword arg, 1) because I think it's obvious for run, and 2) because it mirrors the CLI, which has params as key=val optional positional arguments rather than --param key=val or similar.

dimaqq · 2025-04-14T06:10:59Z

tests/integration/conftest.py


-    yield application
+    yield types.App(app_name)


If the new type encapsulates a single name, why not use a string?

@pytest.fixture(scope="module"): def app_name(...) -> str: ...

Or if the idea is that there may be more fields... maybe return juju.status().apps[app_name] ?

I wondered about that too. However, I think it's better as a proper App type as an app_name fixture to me implies it's very lightweight and just going to return the application name. But this is heavy and deploys a bunch of stuff, so the name "app" and a special name types.App type seems to indicate that intent better.

dimaqq · 2025-04-14T06:14:32Z

tests/integration/test_charm.py

-        test_discourse_srv_status_ok()
+    juju.remove_relation(app.name, "postgresql-k8s:database")
+    juju.wait(
+        lambda status: status.apps[app.name].is_waiting and srv_status_raises_connection_error()


Given that this condition is used twice, what do you feel about something like:

juju.wait(not_ready) def not_ready(status: jubilant.Status) -> bool: if not status.apps[app_name].is_waiting: return False try: ... except (...): return True

Personally I think that's less clear than what I have, but happy for the discourse-k8s team to chime in.

github-actions · 2025-04-14T07:25:52Z

Test results for commit `9745e32`

Test coverage for 9745e32

Name              Stmts   Miss Branch BrPart  Cover   Missing
-------------------------------------------------------------
src/charm.py        373     41     84     15    87%   184, 192-193, 205, 337->345, 378->383, 395, 584-586, 591-592, 604-606, 611-612, 624-626, 649-651, 693->exit, 703->exit, 752-755, 765-766, 790-791, 803-804, 831-833, 835->840, 837-838, 880-881, 897-898, 908->exit, 922
src/database.py      29      1      8      1    95%   56
-------------------------------------------------------------
TOTAL               402     42     92     16    88%

Static code analysis report

Run started:2025-04-14 07:23:20.504418

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 2421
  Total lines skipped (#nosec): 2
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 3

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

gregory-schiano · 2025-04-14T07:13:52Z

tests/integration/test_charm.py

-    test_discourse_srv_status_ok()
-
-
-@pytest.mark.skip(reason="Frequent timeouts")


I think we should port it too, otherwise it'll disappear from the code base, and we might forget about it

gregory-schiano · 2025-04-14T07:18:46Z

tests/integration/test_db_migration.py

@@ -26,90 +23,87 @@ async def test_db_migration(model: Model, ops_test: OpsTest, pytestconfig: Confi
    with a patch but this patch only works for Discourse v3.2.0 and we might
    need to create a new patch for the new version of Discourse.
    """
-    postgres_app = await model.deploy(
+    juju.deploy(
        "postgresql-k8s",


We should put this string in a variable to avoid repeating it everywhere in the test

gregory-schiano · 2025-04-14T07:34:35Z

tests/integration/test_users.py

-    await break_action.wait()
-    assert break_action.status == "failed"
+    with pytest.raises(jubilant.TaskError):
+        juju.run(app.name + "/0", "create-user", {"email": email})


Might be interesting to be able to distinguish between cancelled, error and failed when running a task. Here it relies on raise_on_failure that just checks if not completed

benhoyt added 10 commits April 7, 2025 16:42

test: port integration tests from pytest-operator to Jubilant

1219dfb

fix some dependency and linting issues

27010df

fix typos in test_saml and test_db_migration

be067e6

Merge branch 'main' into jubilant

89c08ac

add --model argument

672f22c

make test_relations wait for connection error

742b8f0

fix nginx-ingress-integrator base; use wait-logging branch for now

a3ae0ce

see if a longer timeout helps

d91eae3

implement abort_on_fail fixture properly

1de7030

update to jubilant 0.4.0; revert some tox.ini stuff

038f811

benhoyt commented Apr 10, 2025

View reviewed changes

benhoyt marked this pull request as ready for review April 10, 2025 04:53

benhoyt requested a review from a team as a code owner April 10, 2025 04:53

benhoyt requested review from jdkandersson and gregory-schiano April 10, 2025 04:53

hmmm, it seems pytest_addoption does have to live here

f2e0a71

gregory-schiano reviewed Apr 11, 2025

View reviewed changes

benhoyt added a commit to benhoyt/jubilant that referenced this pull request Apr 14, 2025

docs: fix and->or in all_* docstrings

e4ba56e

Thanks @gregory-schiano: canonical/discourse-k8s-operator#326 (comment)

benhoyt mentioned this pull request Apr 14, 2025

docs: fix and->or in all_* docstrings canonical/jubilant#96

Merged

benhoyt added a commit to canonical/jubilant that referenced this pull request Apr 14, 2025

docs: fix and->or in all_* docstrings (#96)

2840280

Thanks @gregory-schiano: canonical/discourse-k8s-operator#326 (comment)

benhoyt added 2 commits April 14, 2025 17:10

pack the charm; other tweaks per review

dcb2ef7

get rid of silly bandit warnings

cef3d57

dimaqq reviewed Apr 14, 2025

View reviewed changes

benhoyt added 2 commits April 14, 2025 19:07

tweak to kick CI

7ff0cc2

tweaks based on review

9745e32

gregory-schiano reviewed Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: port integration tests from pytest-operator to Jubilant #326

test: port integration tests from pytest-operator to Jubilant #326

benhoyt commented Apr 7, 2025 •

edited

Loading

benhoyt Apr 10, 2025

arturo-seijas Apr 10, 2025

gregory-schiano Apr 11, 2025

benhoyt Apr 14, 2025

benhoyt Apr 10, 2025

benhoyt Apr 10, 2025

gregory-schiano Apr 14, 2025

benhoyt Apr 10, 2025

gregory-schiano left a comment

gregory-schiano Apr 11, 2025

gregory-schiano Apr 11, 2025

benhoyt Apr 14, 2025

dimaqq Apr 14, 2025

gregory-schiano Apr 11, 2025

benhoyt Apr 14, 2025

gregory-schiano Apr 11, 2025

benhoyt Apr 14, 2025

gregory-schiano Apr 11, 2025

benhoyt Apr 14, 2025

gregory-schiano Apr 11, 2025

benhoyt Apr 14, 2025

dimaqq Apr 14, 2025

dimaqq Apr 14, 2025

benhoyt Apr 14, 2025

dimaqq Apr 14, 2025

benhoyt Apr 14, 2025

dimaqq Apr 14, 2025

benhoyt Apr 14, 2025

github-actions bot commented Apr 14, 2025

gregory-schiano Apr 14, 2025

gregory-schiano Apr 14, 2025

gregory-schiano Apr 14, 2025



		@fixture(scope="module", name="app_config")
		@pytest.fixture(scope="session", name="app_config")

		test_discourse_srv_status_ok()


		@pytest.mark.skip(reason="Frequent timeouts")



		@pytest.fixture(autouse=True)
		def abort_on_fail(request: pytest.FixtureRequest):

	task = juju.run(app.name + "/0", "create-user", {"email": email})
	task = juju.run(app.name + "/0", "create-user", params={"email": email})

test: port integration tests from pytest-operator to Jubilant #326

Are you sure you want to change the base?

test: port integration tests from pytest-operator to Jubilant #326

Conversation

benhoyt commented Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregory-schiano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 14, 2025

Test results for commit 9745e32

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt commented Apr 7, 2025 •

edited

Loading

Test results for commit `9745e32`