LLT-5683: Increase constrains in link state testcases #909

dfetti · 2024-10-29T07:53:24Z

Previously the link state testcase scenarios were structured like setup, do some interaction between peers, sleep for a while and check the reported link states. This approach did not consider the timings of the events.

Proposed solution consists in checking the state constantly during the scenario, not only at the end of it.

☑️ Definition of Done checklist

Commit history is clean (requirements)
README.md is updated
Functionality is covered by unit or integration tests

Previously the link state testcase scenarios were structured like setup, do some interaction between peers, sleep for a while and check the reported link states. This approach did not consider the timings of the events. Proposed solution consists in checking the state constantly during the scenario, not only at the end of it. Signed-off-by: Daniel Fetti <[email protected]>

tomasz-grz · 2025-02-12T09:18:23Z

nat-lab/tests/telio.py

+    async def notify_link_state(self, public_key: str, states: List[LinkState]) -> None:
+        while True:
+            peer = self.get_peer_info(public_key)
+            if peer and peer.link_state in states:


Would this return for any first matching state?

get_peer_info only returns the last event. So this waits until the last event is the specified states. Not sure why it's a List though, to match multiple event types?

LukasPukenis · 2025-02-12T09:44:27Z

nat-lab/tests/test_events_link_state.py

+                        )
+                    ),
+                ],
+                timeout=25,


Just noting that pytest.raises(asyncio.TimeoutError) are good to be left with an explicit timeout value. On Windows which is producing delays this is still gonna pass and on containers that have no delays it will assert for correctness(things don't happen earlier than expected)

LukasPukenis · 2025-02-12T09:45:42Z

nat-lab/tests/test_events_link_state.py

-        # 1 down event when Connecting, 1 up event when Connected
-        assert alpha_events == [LinkState.DOWN, LinkState.UP]
-        assert beta_events == [LinkState.DOWN, LinkState.UP]
+            await client_alpha.wait_for_link_state(beta.public_key, [LinkState.UP], 1)


Just putting in writing what we already talked about wait_for_link_state should have no explicit timeout set or else it will flake on Windows test runners :/

LukasPukenis · 2025-02-12T09:50:22Z

nat-lab/tests/test_events_link_state.py

-        assert beta_events == [LinkState.DOWN, LinkState.UP, LinkState.DOWN]
+        # Expect the link down event
+        # It should arrive in 11-15 seconds after the link is cut and ping mod disabled
+        # And 22-25 seconds if the ping mod is enabled


I am mentioning for general context and also because these comments are a bit ambiguous since the values are not exact but rather ranges which might be confusing now looking at it :)

Link detection is based on 10seconds which is a guarantee from WireGuard(passive-keepalive) and additional configurable RTT - see _generate_setup_parameter_pair() which now is 1 so 11 second in total.

If enhanched detection is used then icmp echo aka ping is also used additionally before reporting the state which adds some extra time(forgot how much).

tomasz-grz · 2025-02-12T10:14:14Z

nat-lab/tests/telio.py

+    async def notify_link_state(self, public_key: str, states: List[LinkState]) -> None:
+        while True:
+            peer = self.get_peer_info(public_key)
+            if peer and peer.link_state in states:


get_peer_info only returns the last event. So this waits until the last event is the specified states. Not sure why it's a List though, to match multiple event types?

tomasz-grz · 2025-02-12T11:22:32Z

nat-lab/tests/test_events_link_state.py


-        await asyncio.sleep(25)
-        alpha_events = client_beta.get_link_state_events(alpha.public_key)


Maybe we would still want to call get_link_state_events() right after notify_link_state() just to make sure we have the correct sequence of events [LinkState.DOWN, LinkState.UP, LinkState.DOWN]?

tomasz-grz · 2025-02-12T11:23:35Z

nat-lab/tests/test_events_link_state.py

-            beta_events = client_alpha.get_link_state_events(beta.public_key)
-
-            # The connection is normal and events should be: initial down, then several up events but no more down events
-            assert alpha_events.count(LinkState.DOWN) == 1


Personally I think checking the count in tests is really good practice

`notify_link_state()` method is doing a busy loop. Instead we can use synchronization primitives to make the code event driven and remove unnecessary sleep.

tomasz-grz · 2025-02-12T16:18:36Z

nat-lab/tests/test_events_link_state.py

+                [
+                    asyncio.create_task(
+                        client_alpha.wait_for_new_link_state(
+                            beta.public_key, [LinkState.DOWN]


I guess technically we should check for [LinkState.DOWN, LinkState.UP] since there should be no new events at all?

tomasz-grz · 2025-02-12T16:22:59Z

nat-lab/tests/test_events_link_state.py

-        await asyncio.sleep(20)
-        alpha_events = client_beta.get_link_state_events(alpha.public_key)
-        beta_events = client_alpha.get_link_state_events(beta.public_key)
+        await client_alpha.wait_for_new_link_state(beta.public_key, [LinkState.UP])


This could also be some shared condition for every test case to follow DRY principles

tomasz-grz · 2025-02-12T16:24:33Z

nat-lab/tests/test_events_link_state.py

@@ -168,18 +190,22 @@ async def test_event_link_state_peers_exchanging_data_for_a_long_time(
            conn.connection for conn in env.connections
        ]

+        await client_alpha.wait_for_new_link_state(beta.public_key, [LinkState.UP])
+        await client_beta.wait_for_new_link_state(alpha.public_key, [LinkState.UP])
+
        for _ in range(0, 40):
            await asyncio.sleep(1)


40 seconds of sleep 🛌

dfetti requested a review from a team as a code owner October 29, 2024 07:53

dfetti force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from e02a96d to 4b5ef12 Compare October 29, 2024 07:54

dfetti temporarily deployed to Internal October 29, 2024 07:54 — with GitHub Actions Inactive

dfetti force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from 4b5ef12 to c1193b5 Compare December 18, 2024 07:55

dfetti had a problem deploying to Internal December 18, 2024 07:55 — with GitHub Actions Error

dfetti force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from c1193b5 to e93f34c Compare January 6, 2025 10:54

dfetti temporarily deployed to Internal January 6, 2025 10:54 — with GitHub Actions Inactive

dfetti force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from e93f34c to 0ab9458 Compare January 6, 2025 10:55

dfetti temporarily deployed to Internal January 6, 2025 10:55 — with GitHub Actions Inactive

dfetti force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from 0ab9458 to 8aaa4d9 Compare January 6, 2025 16:18

dfetti temporarily deployed to Internal January 6, 2025 16:18 — with GitHub Actions Inactive

LukasPukenis force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from 8aaa4d9 to e2980ee Compare February 7, 2025 09:21

LukasPukenis temporarily deployed to Internal February 7, 2025 09:21 — with GitHub Actions Inactive

tomasz-grz self-assigned this Feb 12, 2025

tomasz-grz reviewed Feb 12, 2025

View reviewed changes

LukasPukenis reviewed Feb 12, 2025

View reviewed changes

tomasz-grz changed the title ~~Increase constrains in link state testcases~~ LLT-5683: Increase constrains in link state testcases Feb 12, 2025

tomasz-grz reviewed Feb 12, 2025

View reviewed changes

Remove explicit timeouts

36bd540

tomasz-grz temporarily deployed to Internal February 12, 2025 13:50 — with GitHub Actions Inactive

tomasz-grz temporarily deployed to Internal February 12, 2025 14:35 — with GitHub Actions Inactive

tomasz-grz force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from 35337a7 to 1051cc3 Compare February 12, 2025 15:14

tomasz-grz temporarily deployed to Internal February 12, 2025 15:14 — with GitHub Actions Inactive

Refactor notify_link_state method to be event driven

7c1d372

`notify_link_state()` method is doing a busy loop. Instead we can use synchronization primitives to make the code event driven and remove unnecessary sleep.

tomasz-grz force-pushed the LLT-5683-increase-constranins-in-link-state-testcases branch from 1051cc3 to 7c1d372 Compare February 12, 2025 15:25

tomasz-grz temporarily deployed to Internal February 12, 2025 15:25 — with GitHub Actions Inactive

tomasz-grz reviewed Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLT-5683: Increase constrains in link state testcases #909

LLT-5683: Increase constrains in link state testcases #909

dfetti commented Oct 29, 2024 •

edited

Loading

tomasz-grz Feb 12, 2025

tomasz-grz Feb 12, 2025

LukasPukenis Feb 12, 2025

LukasPukenis Feb 12, 2025

LukasPukenis Feb 12, 2025

tomasz-grz Feb 12, 2025

tomasz-grz Feb 12, 2025

tomasz-grz Feb 12, 2025

tomasz-grz Feb 12, 2025 •

edited

Loading

tomasz-grz Feb 12, 2025 •

edited

Loading

tomasz-grz Feb 12, 2025


		await asyncio.sleep(25)
		alpha_events = client_beta.get_link_state_events(alpha.public_key)

LLT-5683: Increase constrains in link state testcases #909

Are you sure you want to change the base?

LLT-5683: Increase constrains in link state testcases #909

Conversation

dfetti commented Oct 29, 2024 • edited Loading

☑️ Definition of Done checklist

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

tomasz-grz Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

tomasz-grz Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfetti commented Oct 29, 2024 •

edited

Loading

tomasz-grz Feb 12, 2025 •

edited

Loading

tomasz-grz Feb 12, 2025 •

edited

Loading