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

impr: Improve reliability of test #1015

Closed

Conversation

WilliamBergamin
Copy link
Contributor

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

  • Is this something we want to add to this project?
  • Are there other avenues we may want to look into?

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

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.

@seratch
Copy link
Member

seratch commented Jan 18, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

  • python 3.6 -> 23.77s
  • python 3.11 -> 23.41s

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

@WilliamBergamin
Copy link
Contributor Author

@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
Copy link
Member

seratch commented Jan 19, 2024

@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 WilliamBergamin mentioned this pull request Jan 19, 2024
8 tasks
@WilliamBergamin
Copy link
Contributor Author

Closing in favor of #1017

@WilliamBergamin
Copy link
Contributor Author

Closing in favor of #1017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants