feat: improve test speed #1017

WilliamBergamin · 2024-01-19T21:58:39Z

Flow up to the discussion started in #1015

This PR aims to improvement on the mock_web_api_server by making it more robust and attempts to reduce the time spent sleeping in test ("Run tests without aiohttp")

This is achieved by incorporating a solution that allows for waiting up to a specified duration (X amount of time) to confirm the receipt of requests, instead of waiting for a fixed period and then confirming the reception of requests.

Previously, the test located in "Run tests without aiohttp" required 93 seconds for execution. With these changes, the tests can now be completed in just 24 seconds, resulting in a nearly fourfold reduction in wait time.

Similar changes can be applied to asynchronous test and similar reduction in wait time can also be expected for this as well

NOTE: adding content length header to the mock_web_api_server drastically improved request reliability

Before

After

Feedback

The code can be made clearer and there may be some changes that are not necessary, please point out anything that seems relevant

Is this approach worth implementing?

Category (place an `x` in each of the `[ ]`)

slack_bolt.App and/or its core components
slack_bolt.async_app.AsyncApp and/or its core components
Adapters in slack_bolt.adapter
Document pages under /docs
Others

Requirements (place an `x` in each `[ ]`)

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run ./scripts/install_all_and_run_tests.sh after making the changes.

codecov · 2024-01-19T22:03:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69e033f) 91.76% compared to head (7af76f9) 91.76%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1017   +/-   ##
=======================================
  Coverage   91.76%   91.76%           
=======================================
  Files         181      181           
  Lines        6312     6312           
=======================================
  Hits         5792     5792           
  Misses        520      520

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

seratch

THIS IS FANTASTIC! Thank you so much for drastically improving the speed and the stability too! We definitely should go with this approach!

seratch · 2024-01-20T00:06:34Z

tests/adapter_tests_async/test_async_asgi.py

@@ -221,4 +221,4 @@ async def test_url_verification(self):

        assert response.status_code == 200
        assert response.headers.get("content-type") == "application/json;charset=utf-8"
-        assert_auth_test_count(self, 1)
+        assert_auth_test_count(self, 0)


Indeed, this should be zero, but perhaps this had been wrong due to the current test runner mechanism's behavior, right? Nice improvement 👍

Yess, I suspect this may be caused by the current test runner mechanism's behavior but I need to dig into each individual test to see what the expected behavior should be and why these values are different, there seem to be a few like these 🤔

tests/mock_web_api_server.py

seratch · 2024-01-20T00:08:44Z

tests/mock_web_api_server.py

                self.wfile.write("OK".encode("utf-8"))
                return

-            if path == "/received_requests.json":
+            if path == "/health":


Are you using this endpoint? If we no longer need this, you can simply remove it.

Good point 💯 I did add this to try and see if it would be more reliable to ensure the mock_web_api_server responds with a success response before running the tests, there was no notable difference here

seratch · 2024-01-20T00:11:44Z

tests/mock_web_api_server.py

+        self.received_requests = {}
+
+    def get(self, key, default=None):
+        while not self.queue.empty():


This is quite a wise approach 👍

tests/mock_web_api_server.py

seratch · 2024-01-20T00:15:04Z

tests/mock_web_api_server.py

@@ -253,9 +257,22 @@ def stop(self):
        self.join()


+class ReceivedRequestsHandler:


this is a matter of preference but this class does not handle anything but it just stores the received requests. If I named this, it would be a simpler one such as "ReceivedRequests" or "SavedReceivedRequests"

seratch · 2024-01-20T00:15:48Z

tests/mock_web_api_server.py

 def setup_mock_web_api_server(test: TestCase):
    test.server_started = threading.Event()
-    test.thread = MockServerThread(test)
+    test.received_requests_handler = ReceivedRequestsHandler(Queue())


For the same reason I mentioned above, test.received_requests.get("/auth.test") sounds more natural to me.

seratch · 2024-01-20T00:16:36Z

tests/scenario_tests/test_app.py

@@ -77,7 +74,7 @@ class TestExecutor(Executor):

    def test_valid_single_auth(self):
        app = App(signing_secret="valid", client=self.web_client)
-        assert app != None
+        assert app is not None


True, but I think I'll remove these changes in order to simplify the PR, I can do this in a follow up

seratch · 2024-01-20T00:17:14Z

tests/scenario_tests/test_events_ignore_self.py

@@ -38,9 +39,9 @@ def handle_app_mention(say):
        response = app.dispatch(request)
        assert response.status == 200
        assert_auth_test_count(self, 1)
-        sleep(1)  # wait a bit after auto ack()
+        sleep(0.5)  # wait a bit after auto ack()


Is this still necessary?

Yess this is still required since we are asserting that an endpoint was not executed
If we validate without waiting we may assert the request was not sent before it gets sent

Oh, I see. that makes sense

Co-authored-by: Kazuhiro Sera <[email protected]>

…amBergamin/bolt-python into reduce-sleep-time-in-tests

seratch

Looks great to me 👍 You can merge this PR at any time

seratch · 2024-01-24T22:50:29Z

tests/scenario_tests/test_events_ignore_self.py

@@ -38,9 +39,9 @@ def handle_app_mention(say):
        response = app.dispatch(request)
        assert response.status == 200
        assert_auth_test_count(self, 1)
-        sleep(1)  # wait a bit after auto ack()
+        sleep(0.5)  # wait a bit after auto ack()


Oh, I see. that makes sense

WilliamBergamin · 2024-02-07T15:54:35Z

@seratch I understood why some assert_auth_test_count need to be asserted as being called twice instead of once

The auth test is performed for the bot token and the user token in InstallationStoreAuthorize
Many tests use a variation of this fake MemoryInstallationStore implementation, where a user_token is hard coded into the installation returned by find_installation

I'm assuming the changes regarding the auth_test assertion count now reflect the behavior of bolt python

WilliamBergamin added 2 commits January 19, 2024 16:43

Speed up the tests

92d6166

make all tests pass

f21c770

WilliamBergamin added the discussion label Jan 19, 2024

WilliamBergamin requested a review from seratch January 19, 2024 21:58

WilliamBergamin self-assigned this Jan 19, 2024

seratch reviewed Jan 20, 2024

View reviewed changes

WilliamBergamin mentioned this pull request Jan 22, 2024

impr: Improve reliability of test #1015

Closed

8 tasks

WilliamBergamin and others added 6 commits January 22, 2024 11:43

Update tests/mock_web_api_server.py

60a9b59

Co-authored-by: Kazuhiro Sera <[email protected]>

health endpoint not required

2cadfbb

Update tests/mock_web_api_server.py

93aa4ea

Co-authored-by: Kazuhiro Sera <[email protected]>

Merge branch 'reduce-sleep-time-in-tests' of https://github.com/Willi…

22b4a4a

…amBergamin/bolt-python into reduce-sleep-time-in-tests

Move responses out of class

f2e2246

Improve naming

6b17b6f

seratch approved these changes Jan 24, 2024

View reviewed changes

Merge branch 'main' into reduce-sleep-time-in-tests

1fe4e10

WilliamBergamin marked this pull request as ready for review February 1, 2024 22:21

More time saving

7af76f9

WilliamBergamin added improvement area:sync and removed discussion labels Feb 7, 2024

WilliamBergamin merged commit 91f9ef1 into slackapi:main Feb 7, 2024
12 checks passed

WilliamBergamin deleted the reduce-sleep-time-in-tests branch February 7, 2024 16:36

WilliamBergamin restored the reduce-sleep-time-in-tests branch March 4, 2024 16:39

This was referenced Jul 11, 2024

chore: improve unit test speed #1109

Merged

Sporadic unstable tests due to SimpleHTTPServer performance #781

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve test speed #1017

feat: improve test speed #1017

WilliamBergamin commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading

seratch left a comment

seratch Jan 20, 2024

WilliamBergamin Jan 22, 2024

seratch Jan 20, 2024

WilliamBergamin Jan 22, 2024

seratch Jan 20, 2024

seratch Jan 20, 2024

WilliamBergamin Jan 22, 2024

seratch Jan 20, 2024

WilliamBergamin Jan 22, 2024

seratch Jan 20, 2024

WilliamBergamin Jan 22, 2024 •

edited

Loading

seratch Jan 20, 2024

WilliamBergamin Jan 22, 2024

seratch Jan 24, 2024

seratch left a comment

seratch Jan 24, 2024

WilliamBergamin commented Feb 7, 2024 •

edited

Loading

		@@ -253,9 +257,22 @@ def stop(self):
		self.join()


		class ReceivedRequestsHandler:

feat: improve test speed #1017

feat: improve test speed #1017

Conversation

WilliamBergamin commented Jan 19, 2024 • edited Loading

Before

After

Feedback

Category (place an x in each of the [ ])

Requirements (place an x in each [ ])

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

seratch left a 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

WilliamBergamin Jan 22, 2024 • edited Loading

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

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBergamin commented Feb 7, 2024 • edited Loading

WilliamBergamin commented Jan 19, 2024 •

edited

Loading

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

codecov bot commented Jan 19, 2024 •

edited

Loading

WilliamBergamin Jan 22, 2024 •

edited

Loading

WilliamBergamin commented Feb 7, 2024 •

edited

Loading