bloomberg / pytest-memray

pytest plugin for easy integration of memray memory profiler
https://pytest-memray.readthedocs.io/en/latest/
Apache License 2.0
323 stars 23 forks source link

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

Open godlygeek opened 1 year ago

godlygeek commented 1 year ago

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.

azsh1725 commented 1 year ago

Hi!

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

azsh1725 commented 1 year ago

@godlygeek friendly ping :slightly_smiling_face:

godlygeek commented 1 year ago

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 https://github.com/bloomberg/pytest-memray/blob/ad9b759923f3adc30ca19880c5bf633f5a3469f6/.github/workflows/build.yml#L38-L41 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 https://github.com/bloomberg/pytest-memray/blob/ad9b759923f3adc30ca19880c5bf633f5a3469f6/pyproject.toml#L39-L45 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 commented 1 year ago

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

azsh1725 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/bloomberg/pytest-memray/pull/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 commented 1 year ago

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).