Baf 1260/update dependencies #36
Conversation
WalkthroughThis pull request updates project dependencies to newer versions (Flask 2.2.5, cryptography 44.0.0, pydantic 2.12.5, and others) and introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@requirements.txt`:
- Around line 4-17: The requirements entry "setuptools >= 70.0.0" is too
permissive and allows vulnerable setuptools versions; update that requirement to
"setuptools >= 78.1.1" (or later) to mitigate GHSA-5rjg-fvgr-3xxf /
PYSEC-2025-49 — locate the setuptools line in the requirements list and change
the version specifier accordingly.
In `@tests/controllers/car/test_car_controller.py`:
- Line 300: The c.set_cookie call in test_car_controller.py is using "localhost"
while other set_cookie calls use an empty string; update the inconsistent call
(the c.set_cookie invocation that currently passes "localhost" as the domain) to
use an empty string for the domain so all c.set_cookie(...) usages are
consistent across the test file.
In `@tests/controllers/car/test_get_car_state.py`:
- Line 38: Update the two tests
test_all_clients_waiting_get_responses_when_state_relevant_for_them_is_sent and
test_waiting_for_car_state_for_given_car to use the threadpool_test_client
context manager instead of creating a ThreadPoolExecutor around the standard
Flask test client; specifically, wrap the threaded request logic in "with
threadpool_test_client(self.app.app, TEST_TENANT_NAME) as c:" and use the
returned client c inside the executor tasks (or when spawning threads) so the
Flask request context is correctly managed in threaded scenarios.
In `@tests/controllers/order/test_creating_multiple_orders_at_once.py`:
- Line 100: The test
test_waiting_request_for_new_orders_returns_all_just_created_orders_with_their_default_states
still uses the standard test client with ThreadPoolExecutor; update it to use
the threadpool_test_client context manager like the other updated test by
replacing the test client usage with "with threadpool_test_client(self.app.app,
TEST_TENANT_NAME) as c, ThreadPoolExecutor() as executor:" (ensure
threadpool_test_client is imported/available in the test file) so the concurrent
requests use the threadpool-aware client consistently.
In `@tests/controllers/order/test_get_order_state.py`:
- Line 58: Two tests still use ThreadPoolExecutor with the standard Flask test
client and need the same migration to threadpool_test_client to avoid Flask 2.2+
context cleanup bugs: update
test_all_clients_waiting_get_responses_when_state_relevant_for_them_is_sent and
test_waiting_for_order_state_for_given_order to use the threadpool_test_client
context manager (threadpool_test_client(self.app.app, TEST_TENANT_NAME) as c)
and pass that client object into any worker threads instead of the plain
app.test_client or self.app.app; ensure any places referencing the old client
variable are switched to the context-managed client (c) so threads use the safe
threadpool_test_client throughout.
🧹 Nitpick comments (2)
tests/requirements.txt (1)
2-2: Consider pinning the git dependency for reproducibility.The git URL doesn't specify a commit hash or tag. For reproducible builds, consider pinning to a specific version:
fleet_management_http_client_python @ git+https://github.com/bringauto/fleet-management-http-client-python.git@v1.0.0or using a commit hash.
tests/_utils/api_test.py (1)
9-20: LGTM! Good solution for Flask 2.2+ threading issues.The implementation and documentation clearly explain the purpose. The context manager correctly avoids the problematic cleanup while still providing a familiar API.
Consider adding type hints for clarity:
💡 Optional: Add type hints
+from typing import Generator +from flask.testing import FlaskClient +from flask import Flask + `@contextmanager` -def threadpool_test_client(app, tenant=None): +def threadpool_test_client(app: Flask, tenant: str | None = None) -> Generator[FlaskClient, None, None]:
There was a problem hiding this comment.
Pull request overview
This PR updates project dependencies to newer versions and adapts the test suite for Flask 2.2+ compatibility. The primary motivation is to fix test infrastructure issues introduced by Flask 2.2's changed request context handling when using ThreadPoolExecutor.
Changes:
- Updated major dependencies including Flask (2.1.1 → 2.2.5), cryptography (3.4.8 → 44.0.0), PyJWT (2.3.0 → 2.11.0), and others
- Added
threadpool_test_clientcontext manager to work around Flask 2.2+ context cleanup issues with threading - Updated multiple test files to use the new
threadpool_test_clienthelper when using ThreadPoolExecutor - Changed
sys.executableusage in test arguments for better virtual environment support
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements.txt | Updated 14 dependency versions including Flask, cryptography, PyJWT, httpx, psycopg, pydantic, SQLAlchemy, and others |
| tests/requirements.txt | Fixed coverage version from invalid 2.17 to 7.0.0 |
| tests/_utils/api_test.py | Added threadpool_test_client context manager to handle Flask 2.2+ threading issues |
| tests/script_args/test_args.py | Changed from hardcoded "python" to sys.executable for better portability |
| tests/controllers/order/test_get_order_state.py | Updated 4 test methods to use threadpool_test_client |
| tests/controllers/order/test_creating_multiple_orders_at_once.py | Updated 1 test method to use threadpool_test_client, with a spelling fix |
| tests/controllers/car/test_get_car_state.py | Updated 2 test methods to use threadpool_test_client |
| tests/controllers/car/test_car_action_controller.py | Updated 1 test method to use threadpool_test_client |
| tests/controllers/car/test_car_controller.py | Changed set_cookie domain from "" to "localhost" in one test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/controllers/order/test_creating_multiple_orders_at_once.py
Outdated
Show resolved
Hide resolved
koudis
left a comment
koudis
left a comment
There was a problem hiding this comment.
There are some comments from Coderebbit etc.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@fleet_management_api/app.py`:
- Around line 107-108: The call self.set_cookie("", TENANT_COOKIE_NAME, tenant)
uses positional args that will break with Werkzeug 3.0 (server_name removed);
update the call in app.py (the code path that sets the tenant cookie, e.g. in
the _TestClient or request handling method) to use keyword arguments so the
cookie key and value are explicit and the domain is set intentionally—e.g. call
set_cookie with the cookie name as the first arg and the value as value=..., and
pass domain="" (or domain="localhost" if tests expect that) to make behavior
explicit and forward-compatible with Werkzeug/Flask upgrades.
In `@tests/controllers/order/test_order_state_controller.py`:
- Around line 736-738: Remove the leftover debug print and the invalid query
param: delete the line calling print(c.get("/v2/management/order?test_").json)
in the test in tests/controllers/order/test_order_state_controller.py and
replace the GET call with the intended request (either
c.get("/v2/management/order") or c.get("/v2/management/order?<valid_params>") if
a specific query was meant), then assert on the response instead of printing;
keep the subsequent c.set_cookie("localhost", "tenant", "") as-is.
🧹 Nitpick comments (1)
requirements.txt (1)
13-15: Redundant psycopg dependency lines.
psycopg[binary] == 3.2.13(line 15) is an extras specifier that pulls inpsycopg-binaryas a dependency alongsidepsycopg. Having all three lines means lines 13 and 14 are redundant — line 15 alone would suffice.♻️ Suggested simplification
-psycopg == 3.2.13 -psycopg-binary == 3.2.13 -psycopg[binary] == 3.2.13 +psycopg[binary] == 3.2.13
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
@koudis fixes suggested by 🐇 are fixed. However, SonarQube is reporting a duplication failure on new code (8.7% vs the required ≤3%). This is because the same set_cookie call was updated in multiple test files — the duplication is pre-existing in the test setup code, not something introduced by this PR. The set_cookie change itself was necessary: the Flask 2.1.1 → 2.2.5 upgrade pulls in a newer Werkzeug, which rewrote its test client cookie storage with stricter domain matching. Previously, set_cookie("", "tenant", ...) with an empty domain would still match localhost, but the new version requires an explicit "localhost" domain for cookies to be sent with test requests. |
2 participants
Summary by CodeRabbit
Release Notes
Chores
Tests