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

Cache charmcraft pack container, skip unstable tests except on schedule #18

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

carlcsaposs-canonical
Copy link
Contributor

Copy link
Contributor

@welpaolo welpaolo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

All good! Just a minor to be addressed before merging, but I don't think I need to re-review so I approve!

tox.ini Outdated
commands =
pytest -v --tb native --ignore={[vars]tst_path}unit --log-cli-level=INFO -s {posargs}
pytest -v --tb native --log-cli-level=INFO -s {posargs} {[vars]tests_path}/integration/test_s3_charm.py
Copy link

Choose a reason for hiding this comment

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

Could we just make it a bit more general? like just do {[vars]tests_path}/integration

I mean, I see the use case of someone adding a integration tests and then not running it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that if another test file is added, it should be added to the matrix in ci.yaml. pytest-operator creates a new juju model for each Python test file, so it's more efficient for CI to run each of those models on a separate runner

This is currently the approach we're using in our other repositories

Copy link

Choose a reason for hiding this comment

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

Not really: here for instance

Copy link

@deusebio deusebio Mar 3, 2023

Choose a reason for hiding this comment

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

I believe it makes sense that the [testenv:integration] runs all integrations, other more specific like [testenv:integration-charm] run the specific file.

That's why I was saying that not having the specific file is more consistent. If we develop multiple tests, we can then choose to run them parallel, but we need to create the separated envs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On mysql-operator CI, I'm currently working in the direction of merging the integration tox environments to a single environment: canonical/mysql-operator#109

If you want a separate [testenv:integration] that runs all integrations, feel free to add it. I don't think such an environment should be called by CI

Copy link

@deusebio deusebio Mar 3, 2023

Choose a reason for hiding this comment

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

I understand the scope of the PR, although in this specific case is not producing much improvements since it has only one file. But I believe it HAS value to already have the structure in place for being efficient should we add more files, hence the PR.

Here I'm just saying that it is consistent with other repo to call [testenv:integration] to run all integration environments, that's why I would just remove the filename.

When one creates other files and wants to speed up things, they will create other envs called [testenv:integratoin-{suffix}] and run specific file on them test_{suffix}.py, and change the ci.yaml accordingly with those names. If they do not do that, they will still run all the tests

@carlcsaposs-canonical carlcsaposs-canonical merged commit 21eb360 into main Mar 9, 2023
@carlcsaposs-canonical carlcsaposs-canonical deleted the feature/cache-charmcraft-pack branch March 9, 2023 15:42
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