slackapi / bolt-python

A framework to build Slack apps using Python
https://slack.dev/bolt-python/
MIT License
1.06k stars 245 forks source link

impr: Improve reliability of test #1015

Closed WilliamBergamin closed 8 months ago

WilliamBergamin commented 8 months ago

This daft PR aims to improve the reliability of tests by replacing the HTTPServer and SimpleHTTPRequestHandler with bjoern (a light weight C based WSGI web server) and a minimal implementation of a WSGI receiver

The goal is to reduce errors cause by HTTPServer not being fast enough to handle tests and ultimately speed up our unit tests.

This does come with some drawbacks, notably bjoern requires the C dependency libev in order to run.

Feedback

NOTE: this is a prototype and some work is still required

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

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.

seratch commented 8 months ago

Wow, this is interesting! I learnt the library this time. Indeed, the built-in HTTP server can sometimes be slow yet reliable. If we can make the test execution much faster with this change, that's a huge improvement.

One thing I am wondering is that, I know this is still a work-in-progress, but I noticed only a minor speed increase in the main test execution part named "Run tests without aiohttp" — 96 seconds vs 90 seconds. Of course, it's an improvement! However, being less than 10% does not sound so convincing to me. I am unsure if adding libev installation just for this purpose is a fair tradeoff.

Do you anticipate more significant improvements with further optimization?

WilliamBergamin commented 8 months ago

However, being less than 10% does not sound so convincing to me.

@seratch yes that is true, currently the tests are not running much faster then they were before, I believe this is caused by the sleep() statements present in the majority of our test cases. My assumption is that these sleep() statements we added in order to allow the built-in HTTP server to properly handle and process requests.

Ultimately the real speed up would come from minimizing/removing the sleep() statements in the unit tests, the bjoern library should be able to handle request in a more fault tolerant way

Let me know what you think of this and if you believe this may be wrong

WilliamBergamin commented 8 months ago

In commit 097dd81 I've minimized the time spent sleeping in "Run tests without aiohttp" and was able to reliably execute tests locally, the CI pipeline executed the test in following time

Based on this, the potential gain could be up to 75%

WilliamBergamin commented 8 months ago

@seratch we may be able to simply minimize the sleep() statement without introducing these changes and a new library, from my local testing we could cut test time by half without hitting notable stability issues

seratch commented 8 months ago

@WilliamBergamin Optimizing the sleep duration makes sense! One thing I wanted to point out is that, indeed, sleep for 1 second could be too long for many tests but occasionally failing tests due to short sleep time could be frustrating. Thus, you don't need to drastically reduce the time like from 1.0 to 0.1/0.2. Even if 0.2 works well, you can go with 0.3 or 0.4 for safety.

It seems adjusting sleep operations could be a separate and simpler PR, which does not immediately bring the mock server replacement. Could you come up with a different PR for the sleep changes? We can merge it quickly as a short term improvement.

WilliamBergamin commented 8 months ago

Closing in favor of #1017

WilliamBergamin commented 8 months ago

Closing in favor of #1017