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

fix: drop asynctest #2566

Merged
merged 33 commits into from
Oct 28, 2023
Merged

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Oct 25, 2023

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 the IsolatedAsyncioTestCase and AsyncMock objects now included in the builtin unittest 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):

  • For all files using CoroutineMock, import it from aries_cloudagent.tests.mock instead of unittest.mock or asynctest.mock.
  • For all files containing AsyncTest:
    • Replace AsyncTest in the class bases with IsolatedAsyncioTestCase
    • Replace async def setUp to async def asyncSetUp
  • Watch the warnings printed by pytest carefully to see if the updated tests emit a "such_and_such" was never awaited warning. This should be fixed:
    • Enable tracemalloc (I added -e PYTHONTRACEMALLOC=5 to the command in scripts/run_tests)
    • Using the traceback, locate the coroutine that wasn't awaited. Usually this is the result of a wonky mock. More completely mocking and setting the return values of async mocks should help.

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.

@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 26, 2023

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.

@dbluhm dbluhm force-pushed the chore/drop-asynctest branch from 5886c15 to cfc2b8c Compare October 27, 2023 01:32
@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 27, 2023

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.

@dbluhm dbluhm marked this pull request as ready for review October 27, 2023 19:30
@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 27, 2023

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.

@swcurran
Copy link
Contributor

Yeoman work, @dbluhm — thank you. That was clearly a lot!

@swcurran
Copy link
Contributor

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?

@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 27, 2023

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.

@dbluhm dbluhm force-pushed the chore/drop-asynctest branch from fb98918 to baa72c0 Compare October 27, 2023 20:25
@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 27, 2023

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.

@dbluhm dbluhm added this to the 0.11.0 milestone Oct 27, 2023
@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 27, 2023

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 <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@usingtechnology
Copy link
Contributor

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 ruff thing and I just need to figure out the path configuration.

Run: poetry run pytest

see messages like: FAILED aries_cloudagent/wallet/tests/test_util.py::ruff - FileNotFoundError: /home/vscode/.local/bin/ruff

In the devcontainer ruff is located:

/usr/local/lib/python3.9/site-packages/ruff
/usr/local/bin/ruff

Run: ./scripts/run_tests works perfectly. Awesome work. Great knowing that all the tests are properly executing.

Copy link
Contributor

@usingtechnology usingtechnology left a 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.

@dbluhm dbluhm merged commit 8a7e463 into openwallet-foundation:main Oct 28, 2023
8 checks passed
@dbluhm dbluhm deleted the chore/drop-asynctest branch October 28, 2023 01:24
@swcurran swcurran removed the 0.11.0 label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asynctest replacement
3 participants