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

Test with the oldest version of pytest and Memray we support in CI #80

Open
godlygeek opened this issue Jun 19, 2023 · 12 comments
Open
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@godlygeek
Copy link
Contributor

Currently we only test with the newest version of both pytest and Memray, but we declare support for older versions. We should test that pytest-memray really does work with the oldest versions of pytest and Memray that we claim to support.

@godlygeek godlygeek added good first issue Good for newcomers help wanted Extra attention is needed labels Jun 19, 2023
@azsh1725
Copy link

Hi!

I would be happy to help with this issue if you can describe how best to do it.

@azsh1725
Copy link

@godlygeek friendly ping 🙂

@godlygeek
Copy link
Contributor Author

Hey, sorry @azsh1725! I rarely use tox, personally, so I'm not sure what the best way to accomplish this is. I suspect we'll need changes either to

- name: setup test suite ${{ matrix.tox_env }}
run: tox -vv --notest -e ${{ matrix.tox_env }}
- name: run test suite ${{ matrix.tox_env }}
run: tox --skip-pkg-install -e ${{ matrix.tox_env }}
or to https://github.com/bloomberg/pytest-memray/blob/main/tox.ini (or possibly both). Perhaps the best option is adding a new extra like
optional-dependencies.test = [
"covdefaults>=2.2.2",
"pytest>=7.2",
"coverage>=7.0.5",
"flaky>=3.7",
"pytest-xdist>=3.1",
]
called "test-compat" or something like that, and adding new environments to the tox.ini that use extras=test-compat instead of extras=test, and having the build.yml execute those environments in addition to the current ones.

@pablogsal
Copy link
Member

@gaborbernat any pointers on what is the best way to test with multiple memray versions?

@azsh1725
Copy link

@godlygeek Thanks for the answer! I will try to learn how to do this, taking into account your comment and try to help in solving this problem.

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 26, 2023

@gaborbernat any pointers on what is the best way to test with multiple memray versions?

In this case the answer is Constraint files 👍 https://tox.wiki/en/latest/faq.html#using-constraint-files

@gaborbernat
Copy link
Contributor

FWIW even if we test the lower constraints there's no guarantee some versions in between still are compatible. My 2c support only the latest versions of everything 🤷

@godlygeek
Copy link
Contributor Author

FWIW even if we test the lower constraints there's no guarantee some versions in between still are compatible.

That's definitely true, and because of that I think we should test with both the current versions of pytest and Memray, as well as the oldest supported versions of pytest and Memray. It's theoretically possible that some version in the middle was incompatible even though the ones on both ends were compatible, but going forward our CI will detect it if that happens (since any new incompatible version would need to be the latest version at some point, and we'll catch it when testing against the latest version).

@pablogsal
Copy link
Member

pablogsal commented Jun 26, 2023

I think this is enough to do the trick:

diff --git a/tox.ini b/tox.ini
index e41e2c8..16ff519 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,9 +1,10 @@
 [tox]
 envlist =
     py310-cov
-    py310
-    py39
-    py38
+    py38-memray{tip,1.8.1,1.7.0,1.6.0,1.5.0,1.4.1,1.3.1},
+    py39-memray{tip,1.8.1,1.7.0,1.6.0,1.5.0,1.4.1,1.3.1},
+    py310-memray{tip,1.8.1,1.7.0,1.6.0,1.5.0,1.4.1,1.3.1},
+    py311-memray{tip,1.8.1,1.7.0,1.6.0,1.5.0,1.4.1,1.3.1},
     docs
     lint
 requires = tox>=4.2
@@ -28,6 +29,16 @@ allowlist_externals =
     make
 package = wheel
 wheel_build_env = .pkg
+deps =
+    memraytip: https://github.com/bloomberg/memray/archive/main.tar.gz
+    memray1.8.1: memray==1.8.1
+    memray1.7.0: memray==1.7.0
+    memray1.6.0: memray==1.6.0
+    memray1.5.0: memray==1.5.0
+    memray1.4.1: memray==1.4.1
+    memray1.3.1: memray==1.3.1
+    memray1.2.0: memray==1.2.0
+    memray1.1.0: memray==1.1.0

 [testenv:py310-cov]
 commands =

Anything older than 1.3.1 doesn't work on macOS so it would be very annoying to test. We also need to install the memray deps to build from main

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 26, 2023

I think this is enough to do the trick:

Not really. This only covers memray, not all the other packages we're also using, such as pytest. This tests that those memray versions are compatible with latest versions of all other packages, but that's about it. What if pytest 7.0 is not compatible with memray 1.3?

but going forward our CI will detect it if that happens (since any new incompatible version would need to be the latest version at some point, and we'll catch it when testing against the latest version).

This would be true only if we could guarantee that our CI runs once after every single dependency version change, but this is trivially not enough (considering we run the CI once a day) if a dependency gets two release within the same day.

Testing against every combination of dependencies quickly will blow up the CI to a size that's just not reasonable.

Furthermore, ultimately if someone can pull in the latest version of memray they likely can pull in the latest version of pytest-memray; and therefore sufficient to lower pin our dependencies to the latest available versions before every release we do. Ensuring that the latest version of pytest-memray supports an older memray while in theory doable in practice not worth it, IMHO. So my solution here would be to stop declaring support for older memray versions.

@godlygeek
Copy link
Contributor Author

godlygeek commented Jun 27, 2023

This would be true only if we could guarantee that our CI runs once after every single dependency version change, but this is trivially not enough (considering we run the CI once a day) if a dependency gets two release within the same day.

True, but when there's two releases of one package in a day, and we're compatible with the second one but not the first, it's almost certainly because the first introduced an accidental backwards incompatibility, and the second fixed the regression - in which case the first should be yanked, and even if it isn't we'd be better off adding a pytest!=X.Y.Z rather than a pytest>X.Y.Z for that (since we know we're compatible with both what came before and what came after that version).

Testing against every combination of dependencies quickly will blow up the CI to a size that's just not reasonable.

I do agree here. I think it's OK to only test the oldest versions we expect to be able to support and the newest versions, and trust our dependencies to appropriately follow semver and to yank accidentally incompatible versions so that we can trust that if we're compatible with either end, we're good with what's in the middle.

Furthermore, ultimately if someone can pull in the latest version of memray they likely can pull in the latest version of pytest-memray

The reverse isn't necessarily true, though. Think corporate setups where someone might be able to easily update a pure-Python library but where updating a C extension might be a much tougher ask. Or a platform where we don't have Memray wheels, and so they'd need to compile it from source in order to upgrade Memray. Or someone who's using a different pytest plugin that isn't compatible with the latest pytest version, who wouldn't be able to upgrade to the latest pytest-memray if we unnecessarily restricted it to only working with the latest pytest version.

We believe we've never broken backwards compatibility in a Memray release, with the exception of some minor renaming of CLI arguments. I think the only reason we should ever be dropping support for Memray versions in pytest-memray is when pytest-memray itself depends on some feature only provided by new versions of Memray. Which #85 does - after that PR, pytest-memray is definitely not compatible with Memray < 1.7.0 anymore. It would have been nice if CI had flagged that so we knew we needed to update our pyproject.toml accordingly.

@gaborbernat
Copy link
Contributor

gaborbernat commented Jun 27, 2023

The reverse isn't necessarily true, though. Think corporate setups where someone might be able to easily update a pure-Python library but where updating a C extension might be a much tougher ask. Or a platform where we don't have Memray wheels, and so they'd need to compile it from source in order to upgrade Memray.

Until someone explicitly asks for this YAGNI. I disagree trying to support older memray versions, but alas I digress 👍 take it away 😊

https://tox.wiki/en/latest/faq.html#using-constraint-files

is what you want to generally warn for all incompatibilities (assuming semver holds true), @pablogsal solution IMHO is sub-optimal as only test for one of the project dependencies and not all other that are also not pinned (e.g https://github.com/bloomberg/pytest-memray/blob/main/pyproject.toml#L39-L40 and https://github.com/bloomberg/pytest-memray/blob/main/pyproject.toml#L21-L23).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants