Skip to content

Conversation

@WilliamBergamin
Copy link
Contributor

Summary

This PR aims to increase the tests covered by our CI pipeline, this aims to allow dependabot auto merging

Testing

N/A

Category

  • 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

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

codecov bot commented Mar 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.96%. Comparing base (5c7974a) to head (a7035f0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1276   +/-   ##
=======================================
  Coverage   90.96%   90.96%           
=======================================
  Files         222      222           
  Lines        7501     7501           
=======================================
  Hits         6823     6823           
  Misses        678      678           

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I'm glad to know more tests are running more often 🙏 ✨

I'm curious if you were thinking about later introducing codecov? The various jobs running makes this seem like an interesting enhancement if so!

Comment on lines +71 to +73
- name: Run tests for HTTP Mode adapters (WSGI)
run: |
pytest tests/adapter_tests/wsgi/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to match the other test setups, but I'm wondering if this warning is expected or not?

../../../../../opt/hostedtoolcache/Python/3.13.2/x64/lib/python3.13/site-packages/_pytest/config/__init__.py:1441
  /opt/hostedtoolcache/Python/3.13.2/x64/lib/python3.13/site-packages/_pytest/config/__init__.py:1441: PytestConfigWarning: Unknown config option: asyncio_mode
  
    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

🔗 https://github.com/slackapi/bolt-python/actions/runs/13931970838/job/38991049073#step:16:23

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 💯 we can ignore this warning since it comes from trying to configure pytest-asyncio when it is not installed

Comment on lines +88 to +90
- name: Install all dependencies
run: |
pip install -r requirements/testing.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: Should this be with the "Install all dependencies" job?

It's not super clear to me which steps use which dependencies and TBH I wouldn't be against separating sync from async tests if dependencies are shared in unexpected ways 👾

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesss I see what you mean it might be better to do thins in a separate PR 🤔

@WilliamBergamin WilliamBergamin merged commit c0ea531 into main Mar 20, 2025
14 checks passed
@WilliamBergamin WilliamBergamin deleted the improve-ci-coverage branch March 20, 2025 17:18
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.

3 participants