jroachgolf84 opened a new issue, #72277: URL: https://github.com/apache/airflow/issues/72277
## Description > This is an issue that is reserved for the Airflow Summit "Contributors" Workshop. This is denoted with the label `contributors-workshop`. Out of respect for the organizers and participants of this workshop, **please do not implement a PR that addresses this issue.** > > If this issue is still open following Airflow Summit, the label will be removed and the issue can be picked up. One module in the `amazon` provider's auth manager has no dedicated test module: | Module | Expected test file | | --- | --- | | `providers/amazon/src/airflow/providers/amazon/aws/auth_manager/datamodels/login.py` | `providers/amazon/tests/unit/amazon/aws/auth_manager/datamodels/test_login.py` | It is currently suppressed in the `OVERLOOKED_TESTS` allowlist in [`airflow-core/tests/unit/always/test_project_structure.py`](https://github.com/apache/airflow/blob/main/airflow-core/tests/unit/always/test_project_structure.py). This is a scoped subset of the meta issue #35442, limited to the `amazon` auth manager datamodels. ### What should these tests cover? Worth stating the current position honestly, because it affects whether this is worth doing at all. `LoginResponse` is a single field response model, `access_token: str`, on top of the shared `BaseModel`. It is already exercised end to end: `providers/amazon/tests/unit/amazon/aws/auth_manager/routes/test_login.py` drives the login route, which returns `LoginResponse(access_token=token)`, and asserts that `access_token` is present in the JSON body. So this is not a behavioural gap. It is a structural one, and there are two defensible outcomes: 1. Add a small dedicated test asserting that `access_token` is required, is typed as `str`, and serialises to `{"access_token": ...}`. That is cheap, and it does pin something real, since the field name is part of the HTTP contract for the AWS auth manager login endpoint and renaming it would break clients without failing the route test in an obvious way. 2. Decide that a datamodel this thin does not warrant a dedicated module test, leave the allowlist entry in place, and close this issue as "won't do". Option 1 is the lower risk choice, but this is worth a maintainer opinion rather than assuming. One consistency note for whoever decides. The FAB provider has the equivalent module, `providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/datamodels/login.py`, sitting on the same allowlist for the same reason. Whatever is decided here should be applied to both so the two auth managers do not drift. ## Definition of Done 1. Decide between adding a dedicated test and closing as "won't do", and record the reasoning on this issue. 2. If adding it, create the test module at the path in the table above and apply the same decision to the FAB equivalent. 3. Remove the corresponding entry from `OVERLOOKED_TESTS` in `airflow-core/tests/unit/always/test_project_structure.py` only if a test file now exists at that path. 4. This test should pass: ```bash breeze testing providers-tests providers/amazon/tests/unit/amazon/aws/auth_manager ``` --- Drafted-by: Claude Code (Opus 5); reviewed and edited by @jroachgolf84 before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
