Skip to content

chore: fix test warnings across test suite #1844

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.

Sign up for GitHub

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

Open
WilliamBergamin wants to merge 3 commits into
base: main
Choose a base branch
from
+24 −28
Open

chore: fix test warnings across test suite #1844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22ea641
chore: fix test warnings across test suite
WilliamBergamin Mar 20, 2026
c7f3655
Merge branch 'main' into fix-test-warnings
WilliamBergamin Mar 23, 2026
8b3b8cf
improve validation script
WilliamBergamin Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion scripts/format.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ script_dir=`dirname $0`
cd ${script_dir}/..

if [[ "$1" != "--no-install" ]]; then
export PIP_REQUIRE_VIRTUALENV=1
pip install -U pip
pip install -U -r requirements/tools.txt
fi
Expand Down
8 changes: 3 additions & 5 deletions scripts/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@
# ./scripts/install.sh
# Installs all project dependencies (testing, optional, and tools)

set -e

script_dir=$(dirname $0)
cd ${script_dir}/..

pip install -U pip

pip install -U -r requirements/testing.txt \
-U -r requirements/optional.txt \
-U -r requirements/tools.txt
pip install -U -r requirements/testing.txt
pip install -U -r requirements/optional.txt
pip install -U -r requirements/tools.txt
20 changes: 11 additions & 9 deletions scripts/run_validation.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
# all: ./scripts/run_validation.sh
# single: ./scripts/run_validation.sh tests/slack_sdk_async/web/test_web_client_coverage.py

set -e
Copy link
Member

@zimeg zimeg Mar 20, 2026

Choose a reason for hiding this comment

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

🔭 question: Keeping this would cause this script to exit with error code to break CI I thought? Would this be related to deprecation changes otherwise?

Copy link
Contributor Author

@WilliamBergamin WilliamBergamin Mar 23, 2026

Choose a reason for hiding this comment

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

Good question, in CI we don't use this bash script, it's only used by maintainers locally

The script exits with error if it can't install a dependency or run a command, this is problematic when trying to test things locally against python 3.7 since some dependencies like black will fail to install and run.
This allows the scripts to "fail" formatting but still run the unit tests with python 3.7, there might be a better way to do this 🤔

Copy link
Member

@zimeg zimeg Mar 23, 2026

Choose a reason for hiding this comment

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

@WilliamBergamin Might we move this line with -e to before we run the tests but after linting happens?

I understand now this isn't used in CI but might still find it confusing in development - often exit codes help me build confidence 🧰

Copy link
Contributor Author

@WilliamBergamin WilliamBergamin Mar 23, 2026

Choose a reason for hiding this comment

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

I've updated the script so that formatting, linting and typechecking only run when using the latest supported python version and placed -e back into the script

Copy link
Member

@zimeg zimeg Mar 23, 2026

Choose a reason for hiding this comment

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

@WilliamBergamin Thanks for these changes! I like the preference to latest version for formatting so much!


script_dir=`dirname $0`
cd ${script_dir}/..

Expand All @@ -13,16 +11,20 @@ current_py=$(python --version | sed -E 's/Python ([0-9]+\.[0-9]+).*/\1/')

./scripts/install.sh

echo "Generating code ..." && python scripts/codegen.py --path .
echo "Running black (code formatter) ..." && ./scripts/format.sh --no-install

echo "Running linting checks ..." && ./scripts/lint.sh --no-install
set -e

echo "Running tests with coverage reporting ..."
test_target="${1:-tests/}"
PYTHONPATH=$PWD:$PYTHONPATH pytest --cov-report=xml --cov=slack_sdk/ $test_target
echo "Generating code ..." && python scripts/codegen.py --path .

# Run mypy type checking only on the latest supported Python version
if [[ "$current_py" == "$LATEST_SUPPORTED_PY" ]]; then
echo "Running black (code formatter) ..." && ./scripts/format.sh --no-install
echo "Running linting checks ..." && ./scripts/lint.sh --no-install
echo "Running mypy type checking ..." && ./scripts/run_mypy.sh --no-install
else
echo "Skipping formatting, linting, and type checking (current Python: $current_py, required: $LATEST_SUPPORTED_PY)"
fi


echo "Running tests with coverage reporting ..."
test_target="${1:-tests/}"
PYTHONPATH=$PWD:$PYTHONPATH pytest --cov-report=xml --cov=slack_sdk/ $test_target
3 changes: 2 additions & 1 deletion scripts/uninstall_all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ pip uninstall -y slack-sdk
PACKAGES=$(pip freeze | grep -v "^-e" | sed 's/@.*//' | sed 's/\=\=.*//')
# uninstall packages without exiting on a failure
for package in $PACKAGES; do
pip uninstall -y $package
pip uninstall -y $package &
done
wait
Copy link
Member

@zimeg zimeg Mar 20, 2026

Choose a reason for hiding this comment

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

⏳ praise: TIL that not all must be rushed.

10 changes: 6 additions & 4 deletions tests/rtm/test_rtm_client_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
cleanup_mock_web_api_server,
)

websockets_key = web.AppKey("websockets", list) if hasattr(web, "AppKey") else "websockets"


class TestRTMClientFunctional(unittest.TestCase):
def setUp(self):
Expand Down Expand Up @@ -53,7 +55,7 @@ def tearDown(self):

async def mock_server(self):
app = web.Application()
app["websockets"] = []
app[websockets_key] = []
app.router.add_get("/", self.websocket_handler)
app.on_shutdown.append(self.on_shutdown)
runner = web.AppRunner(app)
Expand All @@ -65,16 +67,16 @@ async def websocket_handler(self, request):
ws = web.WebSocketResponse()
await ws.prepare(request)

request.app["websockets"].append(ws)
request.app[websockets_key].append(ws)
try:
async for msg in ws:
await ws.send_json({"type": "message", "message_sent": msg.json()})
finally:
request.app["websockets"].remove(ws)
request.app[websockets_key].remove(ws)
return ws

async def on_shutdown(self, app):
for ws in set(app["websockets"]):
for ws in set(app[websockets_key]):
await ws.close(code=WSCloseCode.GOING_AWAY, message="Server shutdown")

# -------------------------------------------
Expand Down
10 changes: 2 additions & 8 deletions tests/slack_sdk/socket_mode/mock_socket_mode_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,11 @@ def reset_server_state():
self.reset_server_state = reset_server_state

async def health(request: web.Request):
wr = web.Response()
await wr.prepare(request)
wr.set_status(200)
return wr
return web.Response(status=200)

async def disconnect(request: web.Request):
state["disconnect"] = True
wr = web.Response()
await wr.prepare(request)
wr.set_status(200)
return wr
return web.Response(status=200)

async def link(request):
connected = True
Expand Down
Loading