ci: improving readability of testing matrix #244
Conversation
There was a problem hiding this comment.
Thanks for your contributon!
However, the updated matrix is much harder for me to read, which is opposite of "making things easier for contributors" as you claimed. But I am just one data point.
The pytestoldest naming and exact pinning was actually requested by @bsipocz .
The only thing that I 100% agree on right now is adding pytest81: pytest==8.1.* job.
| PY: ${{ matrix.python-version }} | ||
| PT: ${{ matrix.pytest-version }} | ||
| run: | | ||
| # crete the token as "pyXY-test-pytestXY" from the matrix with drop dots |
There was a problem hiding this comment.
The comment has typo and I don't know what "drop dots" mean.
There was a problem hiding this comment.
the base python notation is 3.8 with dot but for env you use just numbers 38
There was a problem hiding this comment.
If I see correctly this matrix update will have way more jobs, as we haven't tested all old versions with all OSes, etc, and have already received criticism that there are way too many jobs for the versions in the middle.
|
p.s. If we deem this PR is ok to go forward, I can approve the CI run then. For now, I will just leave it unapproved. Thank you for your patience! |
you are right, it can be very subjective, but at least I would convert it to d61a564
it is set as explicit |
|
Thanks for the quick response! Let's see what @bsipocz say first. 🐱 |
|
I find the matrix part more legible, but it doesn't do what we had before. Otoh, I don't find the curly brackets and longer lines etc around the one-off combinations helpful, quite the opposite. |
There was a problem hiding this comment.
This has to be changed back. We explicitly use "oldest" in the name, but just here but a lot of other places to show it's the job with the oldest supported version of declared dependencies.
The proposed change is something that let through bugs before we switched to be explicit.
| @@ -13,7 +13,7 @@ setenv = | |||
| numpydev: PIP_EXTRA_INDEX_URL = https://pypi.anaconda.org/scientific-python-nightly-wheels/simple | |||
| description = run tests | |||
| deps = | |||
| pytestoldest: pytest==4.6.0 | |||
| pytest46: pytest==4.6.* | |||
There was a problem hiding this comment.
The oldest have to stay explicit and the version that is declared in the config. Besides that, there is no need to test the latest bugfix in the 4.6 series
|
@bsipocz not sure if I got your reply correctly, sounds like you do not agree with any part of this PR, correct? in such case, let's just close it... :) |
|
Adding |
|
I like the matrix form better but unfortunately it doesn't do the same thing, see a ci run of 19 jobs on main vs 157 jobs in this PR. So if we take away those changes that don't work, unfortunately, it's indeed just the pytest addition that remains in this PR. |
You are right as I did all combinations (and it was a demonstration not necessarily final state), but also you may want to define rich combinations for all Python and |
|
Even that maybe an overkill. The only thing we definitely need to test for is all the (major) pytest versions as those sometimes change suddenly. Then the combination of the OS or python version can be much sparer as pytest itself already test for those. Given the incompatibilities (new pytest on old python vs old pytest on new python) the matrix form will get rather complicated to make it work, and then we'll need to add enough other OS jobs. |
|
I didn't mean to close just yet it with the other PR. |
As it is decalted now it is not trivial to make contributor head what are the tested cases...
So first sing a dictionary written in a single line to its closed-to-table reading which is easier for humans; then rather define rules for run with exceptions then special cases...
This also will increase the number of running suits, but I believe it is in favor of extension stability
cc: @bsipocz @pllim