Cut off nose#1141
Conversation
(or at least, cleaning nose out of all test files!!)
|
This is ready for review and comment. Without review, I will merge this after at least 24 hours, no sooner than Fri 26 Apr 02:00 GMT (Thu 25 Apr 21:00 my local). This is a huge PR in terms of changes, but all of them are individually small and repetitive. I converted legacy |
Thanks for the heads up, I do appreciate them 😀 Will walk through the PR and see how far I get |
sroet
left a comment
There was a problem hiding this comment.
This is a huge PR in terms of changes, but all of them are individually small and repetitive.
This makes it pretty easy to review, and unfortunately also means my many comments are also pretty repetative
To summarize:
Couple question about a test being skipped after it has already completed
Couple style nitpicks
Couple comments about unnecessary backslashes
Couple comments about me preferring no backslashes (feel free to ignore these)
One test case went missing (this is the main one that needs to be changed)
Other things I noticed:
- you sometimes go for
assert xand sometimes forassert x is Truefromassert_equal(x, True), either one is fine by me (while I thinkassert xis technically the better replacement)
Other things I checked:
- I did run a
grep -R "nose" ./on this branch and it only matched a.gitobject
LGTM otherwise
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
dwhswenson
left a comment
There was a problem hiding this comment.
First pass reply; and accepting most (if not all?) suggestions. Then I'll do a second pass to address the rest.
Note to self: commit batch BEFORE finishing review. Otherwise the batch resets? Why, GitHub? Why? Co-authored-by: Sander Roet <sanderroet@hotmail.com>
dwhswenson
left a comment
There was a problem hiding this comment.
Now for one more pass through to see what points remain unresolved....
|
Changes should be all addressed! Take a look when you can, @sroet, and mark it green if ready! Replies to a couple other points:
Yeah, I agree. First macro I added was (of course)
|
sroet
left a comment
There was a problem hiding this comment.
LGTM! thanks for handling the comments (and the useful note in the batch commit)
Resolves #1140, and will allow us to enable Python 3.12 testing (at least in minimal testing; unsure whether some upstreams are caught up yet).
This is a decent bit of tedious work (which is why it hasn't been done yet.) I can facilitate it with some vim macros, but there will be edge cases, so results from macros need to be manually validated. My guess is that I'll do it in the following steps:
assert_equal(a, b)withassert a == bassert_not_equal(a, b)withassert a != braise SkipTest()withpytest.skip()@raises(...)withwith pytest.raises(...)assert_almost_equal, probably with things fromnumpy.testingtest_helpersthat are built onnosenosefrom the build processAdding Python 3.12 to the CI matrix will come in a later PR.