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

Update pytest and sybil #221

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Conversation

Marenz
Copy link
Contributor

@Marenz Marenz commented Feb 5, 2024

fixes #219

@Marenz Marenz requested a review from a team as a code owner February 5, 2024 12:54
@Marenz Marenz requested a review from shsms February 5, 2024 12:54
@github-actions github-actions bot added part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:pytest Affects the configuration of pytest labels Feb 5, 2024
@Marenz Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Feb 5, 2024
@Marenz
Copy link
Contributor Author

Marenz commented Feb 5, 2024

Skipping release notes for this dependency-only update, let me know if you want notes.

@Marenz
Copy link
Contributor Author

Marenz commented Feb 5, 2024

Not sure what the CIs problem is really

@llucax
Copy link
Contributor

llucax commented Feb 6, 2024

I should have mentioned this, but I also had some work done on it, I think I also bumped into tests issues and got distracted with something else and dropped it.

Reading the tests is hard because the tests do generate a whole project for each type of repo, so you have to dig in all the ton of output to look for what it failed.

Besides some weirdness when running git (1, 2) that seemed to be non-fatal (despite some message saying fatal), the real problem seems to be:

INFO: pip is looking at multiple versions of frequenz-repo-config[extra-lint-examples] to determine which version is compatible with other requirements. This could take a while.
ERROR: Cannot install frequenz-actor-test, frequenz-actor-test[dev-flake8,dev-formatting,dev-mkdocs,dev-mypy,dev-noxfile,dev-pylint,dev-pytest]==0.0.post0+d20240205, frequenz-actor-test[dev-mkdocs,dev-noxfile,dev-pytest]==0.0.post0+d20240205 and frequenz-repo-config[extra-lint-examples]==0.0.post1+g3a6a060 because these package versions have conflicting dependencies.

The conflict is caused by:
    frequenz-actor-test[dev-flake8,dev-formatting,dev-mkdocs,dev-mypy,dev-noxfile,dev-pylint,dev-pytest] 0.0.post0+d20240205 depends on pytest==7.4.2; extra == "dev-pytest"
    pytest-asyncio 0.21.1 depends on pytest>=7.0.0
    pytest-mock 3.11.1 depends on pytest>=5.0
    frequenz-actor-test[dev-mkdocs,dev-noxfile,dev-pytest] 0.0.post0+d20240205 depends on pytest==7.4.2; extra == "dev-pytest"
    frequenz-repo-config[extra-lint-examples] 0.0.post1+g3a6a060 depends on pytest<9 and >=8; extra == "extra-lint-examples"

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts

@Marenz Marenz force-pushed the update-pytest-sybil branch from b80e5cb to 0921832 Compare February 6, 2024 13:38
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Feb 6, 2024
@Marenz Marenz enabled auto-merge February 6, 2024 13:40
@Marenz Marenz force-pushed the update-pytest-sybil branch from 0921832 to 8ab7125 Compare February 6, 2024 18:14
@github-actions github-actions bot added the part:template Affects the cookiecutter template files label Feb 6, 2024
@Marenz
Copy link
Contributor Author

Marenz commented Feb 6, 2024

I can't run it successfully locally, but the CI is happy 😆

@llucax
Copy link
Contributor

llucax commented Feb 9, 2024

Seems to work locally for me using nox (so regenerating all nox venvs). It is still running the tests locally, but the dependency issue is not there, it should have happen before running the tests).

Maybe you are using nox -R or just running in your own venv which has the dependencies not fully updated?

Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

Without the suggested change, this would also needs a note in the release notes and upgrade instructions as bumping the dependency is a breaking change (downstream projects will need to upgrade too). But again, locally it works with only widening the dependencies for extra-lint-examples.

Sybil 6 has type information, so we can also remove the mypy exception for it.

Here is a patch of local changes I made that still pass all tests:

diff --git a/cookiecutter/{{cookiecutter.github_repo_name}}/pyproject.toml b/cookiecutter/{{cookiecutter.github_repo_name}}/pyproject.toml
index c603793..d63cd7b 100644
--- a/cookiecutter/{{cookiecutter.github_repo_name}}/pyproject.toml
+++ b/cookiecutter/{{cookiecutter.github_repo_name}}/pyproject.toml
@@ -193,7 +193,7 @@ packages = ["{{cookiecutter.python_package}}"]
 strict = true
 
 [[tool.mypy.overrides]]
-module = ["mkdocs_macros.*", "sybil", "sybil.*"]
+module = ["mkdocs_macros.*"]
 ignore_missing_imports = true
 {%- if cookiecutter.type == "api" %}
 
diff --git a/pyproject.toml b/pyproject.toml
index b396bf2..022ef0d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -63,8 +63,8 @@ lib = []
 model = []
 extra-lint-examples = [
   "pylint >= 2.17.3, < 4",
-  "pytest >= 7.4.2, < 9",
-  "sybil >= 6.0.3, < 7",
+  "pytest >= 7.3.0, < 9",
+  "sybil >= 5.0.3, < 7",
 ]
 dev-flake8 = [
   "flake8 == 6.1.0",
@@ -188,8 +188,6 @@ module = [
   "github_action_utils",
   "mkdocs_macros.*",
   "semver.version",
-  "sybil",
-  "sybil.*",
 ]
 ignore_missing_imports = true
 
diff --git a/tests_golden/integration/test_cookiecutter_generation/actor/frequenz-actor-test/pyproject.toml b/tests_golden/integration/test_cookiecutter_generation/actor/frequenz-actor-test/pyproject.toml
index 0cbe3ee..f12ef54 100644
--- a/tests_golden/integration/test_cookiecutter_generation/actor/frequenz-actor-test/pyproject.toml
+++ b/tests_golden/integration/test_cookiecutter_generation/actor/frequenz-actor-test/pyproject.toml
@@ -163,7 +163,7 @@ packages = ["frequenz.actor.test"]
 strict = true
 
 [[tool.mypy.overrides]]
-module = ["mkdocs_macros.*", "sybil", "sybil.*"]
+module = ["mkdocs_macros.*"]
 ignore_missing_imports = true
 
 [tool.setuptools_scm]
diff --git a/tests_golden/integration/test_cookiecutter_generation/api/frequenz-api-test/pyproject.toml b/tests_golden/integration/test_cookiecutter_generation/api/frequenz-api-test/pyproject.toml
index 27cf961..904116b 100644
--- a/tests_golden/integration/test_cookiecutter_generation/api/frequenz-api-test/pyproject.toml
+++ b/tests_golden/integration/test_cookiecutter_generation/api/frequenz-api-test/pyproject.toml
@@ -156,7 +156,7 @@ packages = ["frequenz.api.test"]
 strict = true
 
 [[tool.mypy.overrides]]
-module = ["mkdocs_macros.*", "sybil", "sybil.*"]
+module = ["mkdocs_macros.*"]
 ignore_missing_imports = true
 
 [tool.setuptools.package-dir]
diff --git a/tests_golden/integration/test_cookiecutter_generation/app/frequenz-app-test/pyproject.toml b/tests_golden/integration/test_cookiecutter_generation/app/frequenz-app-test/pyproject.toml
index 3234e24..ae162ed 100644
--- a/tests_golden/integration/test_cookiecutter_generation/app/frequenz-app-test/pyproject.toml
+++ b/tests_golden/integration/test_cookiecutter_generation/app/frequenz-app-test/pyproject.toml
@@ -162,7 +162,7 @@ packages = ["frequenz.app.test"]
 strict = true
 
 [[tool.mypy.overrides]]
-module = ["mkdocs_macros.*", "sybil", "sybil.*"]
+module = ["mkdocs_macros.*"]
 ignore_missing_imports = true
 
 [tool.setuptools_scm]
diff --git a/tests_golden/integration/test_cookiecutter_generation/lib/frequenz-test-python/pyproject.toml b/tests_golden/integration/test_cookiecutter_generation/lib/frequenz-test-python/pyproject.toml
index 1a15745..8ae960f 100644
--- a/tests_golden/integration/test_cookiecutter_generation/lib/frequenz-test-python/pyproject.toml
+++ b/tests_golden/integration/test_cookiecutter_generation/lib/frequenz-test-python/pyproject.toml
@@ -159,7 +159,7 @@ packages = ["frequenz.test"]
 strict = true
 
 [[tool.mypy.overrides]]
-module = ["mkdocs_macros.*", "sybil", "sybil.*"]
+module = ["mkdocs_macros.*"]
 ignore_missing_imports = true
 
 [tool.setuptools_scm]
diff --git a/tests_golden/integration/test_cookiecutter_generation/model/frequenz-model-test/pyproject.toml b/tests_golden/integration/test_cookiecutter_generation/model/frequenz-model-test/pyproject.toml
index e61c4be..c163225 100644
--- a/tests_golden/integration/test_cookiecutter_generation/model/frequenz-model-test/pyproject.toml
+++ b/tests_golden/integration/test_cookiecutter_generation/model/frequenz-model-test/pyproject.toml
@@ -163,7 +163,7 @@ packages = ["frequenz.model.test"]
 strict = true
 
 [[tool.mypy.overrides]]
-module = ["mkdocs_macros.*", "sybil", "sybil.*"]
+module = ["mkdocs_macros.*"]
 ignore_missing_imports = true
 
 [tool.setuptools_scm]

pyproject.toml Outdated
Comment on lines 66 to 67
"pytest >= 7.4.2, < 9",
"sybil >= 6.0.3, < 7",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should only widen these to avoid making this a breaking change and still support pytest 7.x (if we are not affected by the breaking changes in 8):

Suggested change
"pytest >= 7.4.2, < 9",
"sybil >= 6.0.3, < 7",
"pytest >= 7.3.0, < 9",
"sybil >= 5.0.3, < 7",

I did some local testing with this and it seems to work.

@llucax
Copy link
Contributor

llucax commented Feb 9, 2024

I think we should add a note to the release notes even if it is not a breaking change, as a new feature, like "Add support for pytest 8.x".

@llucax llucax removed the cmd:skip-release-notes It is not necessary to update release notes for this PR label Feb 9, 2024
@Marenz
Copy link
Contributor Author

Marenz commented Feb 12, 2024

Sybil 6 has type information, so we can also remove the mypy exception for it.
[..]
it works with only widening the dependencies

But only widening implies we should keep the exception so it still works on the lowest version, no?

@llucax
Copy link
Contributor

llucax commented Feb 12, 2024

But only widening implies we should keep the exception so it still works on the lowest version, no?

Yeah, true. I think it would be nice for a smoother transition to first be backwards compatible and we create an issue to remove mypy's exception when we drop support for sybil 5.

@llucax
Copy link
Contributor

llucax commented Feb 19, 2024

@llucax llucax disabled auto-merge February 19, 2024 15:55
@llucax
Copy link
Contributor

llucax commented Feb 19, 2024

I'm thinking that this could be a v0.8.1 release instead, as it is a non breaking change (with the changes proposed in my sub-PR) and it will allow downstream project to upgrade the pytest without having to generate the projects files or go through a time-consuming repo-config migration.

Marenz and others added 4 commits February 20, 2024 10:16
* pytest 8.0.0
* sybil 6.0.3

Signed-off-by: Mathias L. Baumann <[email protected]>
The `pytest` and `sybil` dependencies can be purely widen, supporting
the same oldest versions as before, as we are not affected by the
breaking changes.

This allows us to keep backwards compatibility for downstream project.

Signed-off-by: Leandro Lucarella <[email protected]>
When running tests, we now use a `sybil` version that is properly typed,
so we don't need to exclude it from `mypy` anymore.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
@Marenz Marenz force-pushed the update-pytest-sybil branch from d232f19 to 4b09189 Compare February 20, 2024 09:17
@llucax llucax enabled auto-merge February 20, 2024 10:50
@llucax llucax added this pull request to the merge queue Feb 20, 2024
@llucax llucax removed this pull request from the merge queue due to a manual request Feb 20, 2024
@llucax llucax added this pull request to the merge queue Feb 20, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit e2bae45 Feb 20, 2024
14 checks passed
@Marenz Marenz deleted the update-pytest-sybil branch February 20, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:pytest Affects the configuration of pytest part:template Affects the cookiecutter template files part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump pytest and sybil versions to 8.0.x and 6.0.x respectively
2 participants