refactor: upgrade all integration tests to asyncio async await #667

taddes · 2024-03-23T00:48:54Z

taddes · 2024-03-25T19:35:52Z

sentry output test failure only occurring in CI. Investigating why it raises RuntimeError('Event loop is closed')

b4handjr · 2024-04-17T14:20:15Z

tests/integration/test_integration_all_rust.py

+class TestRustWebPush:
    """Test class for Rust Web Push."""


We can get rid of this class and use pytest fixtures but we can do that later on.

@jrbenny35 absolutely! That's the next ticket once this is done and am very excited to have fixtures over the class. Thanks for chiming in!

pjenvey · 2024-04-18T22:48:28Z

tests/integration/async_push_test_client.py

+            res = await object.__getattribute__(self, "get_notification")(timeout)
+            return res


nit: this might have been used for debugging at some point previously but we might as well reduce the diff (same for the end of the ping method)

Suggested change

res = await object.__getattribute__(self, "get_notification")(timeout)

return res

return await object.__getattribute__(self, "get_notification")(timeout)

Good call @pjenvey . Relatedly, was it in convo with you or @jrconlin that it was suggested the "get_notification" calls be made explicitly instead of in this 'clever' way, since I know when I first saw it and looked at the resultant return values in the tests that it was a bit baffling at first.

pjenvey · 2024-04-18T22:50:11Z

tests/integration/test_integration_all_rust.py

-        yield client.hello()
-        yield client.send_bad_data()
-        yield self.shut_down(client)
+        TestRustWebPush.clear_sentry_queue()


We can probably kill these additional clears in all the sentry test methods with the flake issue resolved, letting tearDown handle it

Suggested change

TestRustWebPush.clear_sentry_queue()

Totally. Will move into the pytest setup/teardown fixture in the next PR so it conforms to pytest and not this unittest artifact.

pjenvey · 2024-04-18T22:54:53Z

tests/integration/test_integration_all_rust.py


        # LogCheck does throw an error every time
-        httpx.get(f"http://localhost:{CONNECTION_PORT}/v1/err/crit", timeout=30)
+        async with httpx.AsyncClient() as httpx_client:
+            await httpx_client.get(f"http://localhost:{CONNECTION_PORT}/v1/err/crit", timeout=30)


I'll note that I doubt we actually need a longer timeout=30 in these sentry tests but this is from the original code, so keeping it is fine.

I'd prefer to keep the 30s timeout, if only because CI can be a bit flakey or slow and I'm fine if we don't accidentally timeout and fail the test. If the client responds faster, all the better.

pjenvey · 2024-04-18T22:57:14Z

tests/integration/test_integration_all_rust.py

        assert sorted(values) == ["ERROR:Success", "LogCheck"]

+    @pytest.mark.order(1)


Is pytest-order still necessary (if not I'm inclined to remove it)?

pjenvey · 2024-04-18T23:02:29Z

tests/pyproject.toml

@@ -41,6 +41,9 @@ add-select = ["D212"]
 # D400 First line should end with a period, doesn't work when sentence spans 2 lines
 add-ignore = ["D105","D107","D203", "D205", "D400"]

+[tool.pytest.ini_options]
+asyncio_mode = "auto"


It sounds like this mode means we don't need to decorate tests with any @pytest.mark.asyncio markers? If so that SGTM: let's get rid of them

Yeah I was on the fence about this. While the auto feature is nice, it's not very explicit and gets into "magic" territory with testing (like other annoying stuff like root-test dir defined fixtures in a conftest that pollute the namespace by magically appearing in tests as arguments with 0 context), which I am not a fan of.

It's not immediately evident that the async event loop pytest-asyncio provides is used unless one goes digging in the config. I know in projects I've worked on when random configs are defined with side effects like that, it can lead to confusion. What do you think? It would be nicer not to have to annotate with so many @pytest.mark.asyncio decorators, yet something about the explicit nature seemed sensible. Definitely on point that we shouldn't have both. WDYT? For now I just took the config out but can switch it around.

👍 I'm fine w/ either way

pjenvey · 2024-04-18T23:08:17Z

tests/integration/test_integration_all_rust.py

+        """Clear any values present in Sentry queue."""
+        while not MOCK_SENTRY_QUEUE.empty():
+            MOCK_SENTRY_QUEUE.get_nowait()
+        return


Suggested change

return

pjenvey · 2024-04-18T23:13:54Z

tests/integration/test_integration_all_rust.py


-    @inlineCallbacks
-    def quick_register(self):
+    @pytest.mark.asyncio


regardless of asyncio auto mode or not, this and shut_down shouldn't be decorated w/ @pytest.mark.asyncio as they're not tests

Thanks, good catch 👍

pjenvey · 2024-04-18T23:37:03Z

tests/integration/test_integration_all_rust.py

+        future that returns the expected float latency value.
+        """
+        client = await self.quick_register()
+        result = await client.ping()


I wasn't familiar with the websockets ping API so this confused me a bit: could we rename this result to pong_waiter like their example, and/or note in the method docstring we're awaiting a pong response.

…tween ci and local

…uture in test

…lt between local and ci

… flaky ci issue with queue

tests/integration/test_integration_all_rust.py

jrconlin · 2024-04-19T19:29:03Z

tests/integration/async_push_test_client.py

        result = await self.ws.recv()
        log.debug(f"Recv: {result}")
        return result

+    async def ping(self):
+        """Test ping/pong request and respose of websocket.
+        A ping may serve as a keepalive or as a check that the remote endpoint received.


Suggested change

A ping may serve as a keepalive or as a check that the remote endpoint received.

A ping may serve as a keepalive or as a check that the remote is continuing to respond to websocket protocol messages.

jrconlin · 2024-04-19T19:30:10Z

tests/integration/async_push_test_client.py

        result = await self.ws.recv()
        log.debug(f"Recv: {result}")
        return result

+    async def ping(self):


Not really sure that this is necessary, since it's testing an aspect of the Websocket protocol, not our implementation of Web Push but 🤷

Yeah, you are right, don't necessarily need it. I can remove

jrconlin · 2024-04-19T19:32:42Z

tests/integration/test_integration_all_rust.py


        # LogCheck does throw an error every time
-        httpx.get(f"http://localhost:{CONNECTION_PORT}/v1/err/crit", timeout=30)
+        async with httpx.AsyncClient() as httpx_client:
+            await httpx_client.get(f"http://localhost:{CONNECTION_PORT}/v1/err/crit", timeout=30)


I'd prefer to keep the 30s timeout, if only because CI can be a bit flakey or slow and I'm fine if we don't accidentally timeout and fail the test. If the client responds faster, all the better.

jrconlin · 2024-04-19T20:32:05Z

Thank you for all the work on this. It's awesome.

taddes · 2024-04-19T21:38:12Z

Thanks so much @jrconlin ❤️

pjenvey · 2024-04-19T23:31:26Z

One last thing I forgot to mention: these changes added a few new warnings to the test suite, we should log an issue to take a pass/attempt to quiet them (if possible)

taddes · 2024-04-20T00:30:26Z

Yes @pjenvey , was going to do just that. It's those intermittent socket disconnection warnings which I think are a byproduct of the new library. I'll file the ticket for that and will look at it. Thanks!

taddes self-assigned this Mar 23, 2024

taddes marked this pull request as draft March 23, 2024 01:07

taddes force-pushed the refactor/SYNC-4120_upgrade_core_test_asyncio branch from 2c2c84a to 9ac75e6 Compare April 1, 2024 14:07

taddes force-pushed the refactor/SYNC-4120_upgrade_core_test_asyncio branch from cafe340 to 6356666 Compare April 8, 2024 18:02

taddes force-pushed the refactor/SYNC-4120_upgrade_core_test_asyncio branch from 2698ca2 to bc577c9 Compare April 16, 2024 17:24

taddes marked this pull request as ready for review April 16, 2024 21:34

taddes requested review from pjenvey and jrconlin April 16, 2024 21:34

b4handjr reviewed Apr 17, 2024

View reviewed changes

taddes force-pushed the refactor/SYNC-4120_upgrade_core_test_asyncio branch from 48f698c to 9413df5 Compare April 18, 2024 20:26

pjenvey requested changes Apr 18, 2024

View reviewed changes

taddes added 18 commits April 19, 2024 14:44

update get_notification call part of send notification to async

65ef465

autouse for asyncio mode for pytest-asyncio test collector event loop

0a2c75b

async await updates for all tests

ec916db

convert httpx calls to async context managers

1302411

verify if one extra value now in queue to solve variance in result be…

4b83ec5

…tween ci and local

update timeout to possibly resolve ci error

333d8de

update to disconnct for client

97c8232

sentry method to clear queue explicitly

ac6737b

additional calls to sentry queue clear

a7d735f

change to moz_ping, add new method to test low level ping and await f…

703ea17

…uture in test

move access of events from queue into context manager. Divergent resu…

9ad20d0

…lt between local and ci

refactor no output to try and fix ci breakage. Passes locally

382b00e

alter timeout

04212fd

add pytest-order to run sentry test first, alter text and add note on…

d2aca0f

… flaky ci issue with queue

added note for test order

3593708

alteration of timeout for queue get to resolve runtime ci flake

c1bbc09

added note context on increased timeout in autoconnect sentry test

c4f89cf

rebase and update lockfile

b723f40

refactor review comments

334f4f8

taddes force-pushed the refactor/SYNC-4120_upgrade_core_test_asyncio branch from 37b27e7 to 334f4f8 Compare April 19, 2024 18:44

taddes requested a review from pjenvey April 19, 2024 18:56

pjenvey requested changes Apr 19, 2024

View reviewed changes

tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved

taddes mentioned this pull request Apr 19, 2024

refactor: Replace Bottle with FastAPI #668

Merged

remove extraneous comment

d7d3958

taddes requested a review from pjenvey April 19, 2024 19:33

jrconlin reviewed Apr 19, 2024

View reviewed changes

remove websocket ping test and asyncio dep from rev comments

1f44987

pjenvey approved these changes Apr 19, 2024

View reviewed changes

jrconlin approved these changes Apr 19, 2024

View reviewed changes

taddes changed the title ~~Upgrade all integration tests to asyncio async await~~ refactor: upgrade all integration tests to asyncio async await Apr 19, 2024

taddes merged commit c7448f4 into master Apr 19, 2024
1 check passed

taddes deleted the refactor/SYNC-4120_upgrade_core_test_asyncio branch April 19, 2024 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: upgrade all integration tests to asyncio async await #667

refactor: upgrade all integration tests to asyncio async await #667

taddes commented Mar 23, 2024 •

edited by jira bot

Loading

taddes commented Mar 25, 2024

b4handjr Apr 17, 2024

taddes Apr 17, 2024

pjenvey Apr 18, 2024

taddes Apr 19, 2024

pjenvey Apr 18, 2024

taddes Apr 19, 2024

pjenvey Apr 18, 2024

jrconlin Apr 19, 2024

pjenvey Apr 18, 2024

pjenvey Apr 18, 2024

taddes Apr 19, 2024

pjenvey Apr 19, 2024

pjenvey Apr 18, 2024

pjenvey Apr 18, 2024

taddes Apr 19, 2024

pjenvey Apr 18, 2024

jrconlin Apr 19, 2024

jrconlin Apr 19, 2024

taddes Apr 19, 2024

jrconlin Apr 19, 2024

jrconlin commented Apr 19, 2024

taddes commented Apr 19, 2024

pjenvey commented Apr 19, 2024

taddes commented Apr 20, 2024

		res = await object.__getattribute__(self, "get_notification")(timeout)
		return res

	res = await object.__getattribute__(self, "get_notification")(timeout)
	return res
	return await object.__getattribute__(self, "get_notification")(timeout)

		assert sorted(values) == ["ERROR:Success", "LogCheck"]

		@pytest.mark.order(1)

	A ping may serve as a keepalive or as a check that the remote endpoint received.
	A ping may serve as a keepalive or as a check that the remote is continuing to respond to websocket protocol messages.

refactor: upgrade all integration tests to asyncio async await #667

refactor: upgrade all integration tests to asyncio async await #667

Conversation

taddes commented Mar 23, 2024 • edited by jira bot Loading

taddes commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrconlin commented Apr 19, 2024

taddes commented Apr 19, 2024

pjenvey commented Apr 19, 2024

taddes commented Apr 20, 2024

taddes commented Mar 23, 2024 •

edited by jira bot

Loading