Skip to content

test: migrate pytest-operator to jubilant#218

Open
yanksyoon wants to merge 37 commits intomainfrom
chore/migrate-to-jubilant
Open

test: migrate pytest-operator to jubilant#218
yanksyoon wants to merge 37 commits intomainfrom
chore/migrate-to-jubilant

Conversation

@yanksyoon
Copy link
Copy Markdown
Member

@yanksyoon yanksyoon commented Apr 16, 2026

Applicable spec:

Overview

  • Migrate pytest-operator to jubilant as a mitigation to bugs with websocket disconnects

Rationale

  • Issues with pytest-operator and juju leads to websocket disconnect errors, blocking CI

Juju Events Changes

Module Changes

Library Changes

Checklist

Only test changes.

yanksyoon and others added 2 commits April 16, 2026 11:36
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s_not_rebuild_image

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@yanksyoon yanksyoon changed the title [WIP] test: migrate pytest-operator to jubilant test: migrate pytest-operator to jubilant Apr 16, 2026
@yanksyoon yanksyoon marked this pull request as ready for review April 16, 2026 03:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Migrates the repository’s integration test harness from pytest-operator/async Juju usage to the jubilant (synchronous) Juju wrapper to reduce CI failures from websocket disconnects.

Changes:

  • Replace pytest-operator + asyncio-based tests/fixtures with jubilant-based synchronous equivalents.
  • Rework integration fixtures to create/manage a temporary Juju model via jubilant.temp_model(), plus add --keep-models for debugging.
  • Update tox and integration requirements to drop async-only dependencies and add jubilant.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tox.ini Replace pytest-operator/asyncio deps with jubilant in lint/integration envs.
tests/integration/types.py Update test config typing from Model to jubilant.Juju.
tests/integration/test_upgrade.py Port upgrade flow to jubilant APIs and synchronous waiting/log checks.
tests/integration/test_charm.py Port integration tests to jubilant APIs and synchronous helpers/context management.
tests/integration/requirements.txt Remove async-specific dependency (nest_asyncio).
tests/integration/helpers.py Convert wait_for/wait_for_images from async to sync utilities.
tests/integration/conftest.py Replace ops_test/model fixtures with a jubilant-managed temporary model + updated deploy/secret/config flows.
tests/conftest.py Add --keep-models pytest CLI option to support jubilant temp model retention.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tox.ini Outdated
Comment thread tests/integration/test_charm.py Outdated
- Use jubilant~=1.0 in lint env for consistent versioning with integration env
- Wrap _change_cronjob_to_minutes yield in try/finally to ensure cron cleanup on test failure

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@florentianayuwono florentianayuwono left a comment

Choose a reason for hiding this comment

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

lgtm thankyou!

yanksyoon and others added 2 commits April 16, 2026 13:35
encoding is only valid with filename, not handlers. The WatchedFileHandler
is already created with encoding='utf-8'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@yhaliaw yhaliaw left a comment

Choose a reason for hiding this comment

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

We need to review the logging of the jubilant first.

@yanksyoon yanksyoon requested a review from yhaliaw April 16, 2026 10:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

tests/integration/conftest.py:410

  • This function uses await inside a non-async def and references app/app.model even though no app variable is defined (the deploy call doesn’t capture a return value). This is a syntax/runtime error; rewrite using jubilant operations (deploy, grant secrets, set config) without await, or make the whole fixture async and restore the asyncio toolchain.
    test_configs.juju.deploy(
        test_configs.charm_file,
        app_name,
        constraints=base_machine_constraint,
        config=app_config,
        log=False,
    )
    await app.model.grant_secret(openstack_password_secret.name, app.name)
    await app.model.grant_secret(script_secret.name, app.name)
    await app.set_config(
        {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/integration/conftest.py
Comment thread .github/workflows/integration_test.yaml Outdated
Comment thread tests/integration/conftest.py Outdated
Comment thread tests/integration/conftest.py
Comment thread tests/integration/conftest.py
yanksyoon and others added 2 commits April 21, 2026 12:29
- Pin integration_test.yaml to operator-workflows@main instead of
  mutable feature branch ref
- Convert openstack_password_secret_fixture from async pytest_asyncio
  to sync pytest fixture using juju.add_secret()
- Convert app_config_fixture from async pytest_asyncio to sync pytest
  fixture (body was already synchronous)
- Fix app_fixture return type (AsyncGenerator[Application] -> Generator[str])
  and replace await calls with jubilant API: grant_secret(), config()
- Fix _prepare_charmhub_app_config: remove async, replace ops_test
  param with juju: jubilant.Juju, fix NameError on test_configs
- Fix app_on_charmhub_fixture: convert to sync pytest fixture, remove
  ops_test param, define missing app_name, replace all async/model
  calls with jubilant API, yield app name string instead of Application

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jubilant.Juju.deploy() does not accept a log parameter (it only
exists on the private _cli method). Remove it from _deploy_test_charm
and app_fixture to avoid TypeError at runtime.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Member Author

@yanksyoon yanksyoon left a comment

Choose a reason for hiding this comment

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

Initial review done

yanksyoon and others added 2 commits April 22, 2026 00:48
…alls

- Flatten parenthesized tuple assignment for _prepare_charmhub_app_config
  to match black's preferred style
- Remove log=False from juju.deploy() in test_cos_agent_relation;
  jubilant's deploy() does not accept a log parameter

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hub_fixture

Was annotated as str instead of dict, causing mypy to error:
  Argument 'constraints' to 'deploy' of 'Juju' has incompatible
  type 'str'; expected 'Mapping[str, str] | None'

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread tests/integration/conftest.py
Comment thread tests/integration/conftest.py
yanksyoon and others added 7 commits April 22, 2026 14:07
self.config.get() returns int | float | str, but Model.get_secret(id=...)
expects str | None. Cast to str explicitly since we already guard on truthiness.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace 'via' with 'through' or 'using' (Canonical.025a)
- Replace 'etc' with 'and so on' (Canonical.025a)
- Fix 'snapstore' -> 'the Snap Store' in changelog (Canonical.000)
- Fix 'github actions' -> 'GitHub Actions' in architecture doc (Canonical.000)
- Fix Ubuntu version format to include LTS suffix (Canonical.003)
- Add 'aproxy' and 'opentelemetry' to vale accept list (product names)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yanksyoon and others added 2 commits April 22, 2026 15:55
… migration

- Add update_status() to image.Observer so block_if_invalid_config works
- Fix test_block_on_image_relation_not_ready to mock BuilderConfig.from_charm
- Add tests for ImageConfig.from_charm, CloudConfig.from_charm,
  BuilderConfig.from_charm, upload_cloud_ids property, _get_num_parallel_build
- Add tests for _parse_openstack_clouds_config with upload auth configs
- Add tests for _parse_openstack_clouds_auth_configs_from_relation with
  units (complete and incomplete data)
- Add test for _parse_script_secrets with no secrets configured
- Add test for _on_image_relation_joined with no upload_cloud_ids

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Inline MagicMock call in test_charm.py to satisfy black formatting
- Change B106 nosec to B105 for dict password string literal in test_state.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yanksyoon and others added 15 commits April 23, 2026 10:03
When upgrading from a charmhub edge revision that predates the
openstack-password-secret config option, the upgrade test was failing
because the old charm had no credentials and the new charm (which
requires openstack-password-secret) also couldn't find them.

Restore the fallback in _prepare_charmhub_app_config to set the legacy
openstack-password config when the charmhub version doesn't yet expose
openstack-password-secret. The constant is kept local to the test code
since state.py no longer needs the legacy option.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pgrade test

The app_fixture in test_upgrade.py was calling juju.refresh() without
granting or setting the openstack-password-secret config. When upgrading
from an older charmhub revision that didn't expose this config option,
the refreshed charm would raise:

  InvalidCloudConfigError: Please supply OpenStack password via openstack-password-secret.

Fix by adding openstack_password_secret to app_fixture's parameters and
explicitly granting the secret and setting the config immediately after
juju.refresh(), ensuring the new charm can read the OpenStack password.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- test_upgrade.py: add sudo to cat /var/log/juju/unit-*.log since the
  ubuntu user doesn't have read permission on juju log files; without it
  juju.ssh() raises CLIError on every poll, propagating immediately out
  of wait_for()
- test_charm.py: wrap logrotate --debug command in sudo bash -c so that
  the 2>&1 redirect is interpreted by a shell, ensuring stderr (where
  logrotate outputs debug info) is merged into stdout and captured by
  juju.ssh()

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants