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

Remove asynctest dependency and fix "coroutine not awaited" warnings #2755

Merged
merged 18 commits into from
Feb 2, 2024

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Feb 2, 2024

As initially done by @dbluhm in #2566, this drops the asynctest dependency, as it's massively out of date and generates hundreds of warnings in the tests - making it harder to improve on other warning logs.

It seems the dependency was subtly reintroduced in #2596. This PR attempts to refactor tests to only use unittest, as they fully support async tests since python 3.8 and external deps for async tests shouldn't be needed.

Edit: Summary of changes:

➖ Drops dependencies: asynctest (deprecated), and async-case (unused)

♻️ Refactors tests to use unittest mocking

✅ Includes fixes for 3 tests that were silently not implemented properly (coroutine not awaited)

👷 Added a condition to tests workflow that it will fail if "coroutine not awaited" appears in the warning logs. (Here's a sample result of a workflow failing, where all tests passed, but the warning appears in logs)

@ff137
Copy link
Contributor Author

ff137 commented Feb 2, 2024

Funny enough, the tests actually report more warnings now ...
Doing a diffcheck I see that the deprecated @coroutine method warnings are indeed gone, but without asynctest there are now warnings displayed for modules in aries_cloudagent/anoncreds which wasn't displaying previously. Maybe asynctest wasn't logging things properly.
They're just Marshmallow warnings. I'll see if I can patch some of those in this PR too

@ff137
Copy link
Contributor Author

ff137 commented Feb 2, 2024

Quite a few different cases of deprecations warnings to fix, so I'll try tackle that in #2756

@ff137
Copy link
Contributor Author

ff137 commented Feb 2, 2024

@dbluhm just a heads up that some of the "coroutine 'AsyncMockMixin._execute_mock_call' was never awaited warnings appear to have been introduced in this PR: #2687

I could fix the one case, in test_accept_response_find_by_thread_id_no_did_doc_attached.
mock_response = mock.MagicMock() was changed to an AsyncMock, which let the test pass but didn't do things properly.

Currently, test_create_request_emit_did_peer_2 and _4 aren't being mocked properly. Seems to be the only other "coroutine never awaited" problem. I'll see if I can figure those out too

Edit: think I figured it out. Please review the latest commits to check out what was missing!

@ff137 ff137 changed the title Remove asynctest dependency Remove asynctest dependency and fix "coroutine not awaited" warnings Feb 2, 2024
@ff137
Copy link
Contributor Author

ff137 commented Feb 2, 2024

I figured it's a good idea to modify the tests workflow to always fail if a coroutine is not awaited ... because it indicates tests weren't set up properly, and it will avoid "silently failing" tests from merging in the future. So, I added that to the tests workflow:

      - name: Tests
        run: |
          poetry run pytest 2>&1 | tee pytest.log
          if grep -Eq "RuntimeWarning: coroutine .* was never awaited" pytest.log; then
            echo "Failure: Detected unawaited coroutine warning in pytest output."
            exit 1
          fi

Suggestions are welcome

@ff137 ff137 marked this pull request as ready for review February 2, 2024 12:49
Signed-off-by: ff137 <[email protected]>
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! The AnonCreds code was on the back of my mind when I did the first round of async test removal but I'd since forgotten it needed attention after merging. Good catch on the did:peer tests as well! Quick comment on the test workflow adjustments.

Comment on lines 31 to 35
poetry run pytest 2>&1 | tee pytest.log
if grep -Eq "RuntimeWarning: coroutine .* was never awaited" pytest.log; then
echo "Failure: Detected unawaited coroutine warning in pytest output."
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is wise to fail tests on coroutine never awaited errors. I have an inkling that pytest may support this directly? Perhaps it just has a "fail on warning" and "don't fail on warning" and doesn't allow granular failures. Will piping to tee preserve the exit code for the pytest invocation? In other words, will the action still fail when there's a test failure and not just on a warning we don't want to see?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I thought the same to config pytest to fail on warning, but afaik it only lets you specify the kind of warning (RuntimeWarning / Deprecation warning) as a catch all. So to fail on specific warnings can be solved with this pipe to file and grep approach.

I also wondered if the pytest exit code is preserved -- thought the 2>&1 logic might accommodate that, but not sure. I'll test and refactor as needed 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbluhm Indeed! It wasn't preserving the exit code ... doh. (Thanks GPT)
Luckily one can use ${PIPESTATUS[0]}, because PIPESTATUS preserves the exit code of the most recent pipeline.
(Also thanks GPT) 😄

Testing fail/success cases now

Copy link

sonarcloud bot commented Feb 2, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@ff137
Copy link
Contributor Author

ff137 commented Feb 2, 2024

Testing new workflow changes:

Checks out 👍

Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again and nice work!

@dbluhm dbluhm merged commit 9ed0fd8 into openwallet-foundation:main Feb 2, 2024
8 checks passed
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.

2 participants