-
Notifications
You must be signed in to change notification settings - Fork 511
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
fix: drop asynctest #2566
fix: drop asynctest #2566
Conversation
Hiccup: AsyncMock returns AsyncMocks when called. CoroutineMock behaved more like an AsyncMock that returns MagicMocks. I'll see if I can provide a simple adapter since this has pretty drastic effects on some tests. |
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
5886c15
to
cfc2b8c
Compare
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
I'm discovering a frightening number of tests that were silently broken. I think the AsyncMock update is making this particular category of issue more obvious thanks to warnings it will emit when the coroutine isn't awaited. I'll continue to work through those warnings. |
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
This PR is now ready for review. It is obviously a big change but the changes should be straightforward enough to follow. In reviewing these changes, I recommend checking out individual commits rather than viewing all files changed at once. Given the size and the fact that this touches basically every single test module in ACA-Py, I'm more than happy to provide support to the authors of pending PRs to guide them through necessary changes or resolving conflicts. |
Yeoman work, @dbluhm — thank you. That was clearly a lot! |
BTW — it appears in a quick glance that there is a single non-test update — made to this file: aries_cloudagent/protocols/discovery/v1_0/manager.py That was intended, I assume? |
It was an intentional change but in retrospect, it probably would have been better to leave it off this PR. The change shouldn't affect anything but better type hints within the changed method. I'll see about striking that change from my commits, just for the sake of keeping everything in this PR strictly about the tests. |
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
fb98918
to
baa72c0
Compare
I did a spot of history rewriting and reverted the change to the discovery v1 manager. This PR should now be exclusively test changes and fixes. |
Doing one last step: removing asynctest as a dependency. Testing locally. I'll push once I've validated that everything looks good. |
Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
Kudos, SonarCloud Quality Gate passed! |
incredible work! i know you don't use the devcontainer, so I will file a follow up ticket to make the tests work within the devcontainer. it's a Run: see messages like: In the devcontainer
Run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 - awesome.
Left a comment about a follow-up devcontainer only ticket but don't feel like that should hold up this PR.
I felt like doing something tedious and mind-numbing (only mostly joking; macros and
:bufdo
/:cfdo
combined with ruff made this go pretty quick) so I took a stab at removing asynctest and replacing it with theIsolatedAsyncioTestCase
andAsyncMock
objects now included in the builtinunittest
package.Fixes #1717
Given that there are other PRs currently open (and others in the works, I'm sure), I'll describe the general process I followed (updated):
CoroutineMock
, import it fromaries_cloudagent.tests.mock
instead ofunittest.mock
orasynctest.mock
.AsyncTest
:AsyncTest
in the class bases withIsolatedAsyncioTestCase
async def setUp
toasync def asyncSetUp
"such_and_such" was never awaited
warning. This should be fixed:-e PYTHONTRACEMALLOC=5
to the command inscripts/run_tests
)There's a relatively small number of failing tests right now so opening as draft but I wanted to put this on the radar.No failing tests but still working through warnings.