Skip to content

Reorganize docs tests for task-sdk into task-sdk/tests/docs#51876

Merged
potiuk merged 1 commit intoapache:mainfrom
astronomer:move-task-sdk-docs-tests-docs
Jun 23, 2025
Merged

Reorganize docs tests for task-sdk into task-sdk/tests/docs#51876
potiuk merged 1 commit intoapache:mainfrom
astronomer:move-task-sdk-docs-tests-docs

Conversation

@sunank200
Copy link
Copy Markdown
Collaborator

@sunank200 sunank200 commented Jun 18, 2025

This is follow up PR based on this comment #51153 (comment)

This PR refactors our tests for task-sdk by moving all docs-related tests into a dedicated docs directory.

related: #51519


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@sunank200 sunank200 requested review from ashb and kaxil and removed request for ashb and kaxil June 18, 2025 08:59
@sunank200 sunank200 force-pushed the move-task-sdk-docs-tests-docs branch from 8f5dc4f to 43967ed Compare June 18, 2025 09:00
@potiuk
Copy link
Copy Markdown
Member

potiuk commented Jun 18, 2025

I would even propose to turn them into scripts and run them via pre-commit. Those tests are fast and can be easily run every time "task-sdk" changes and they are much more like "static" tests than "unit tests"

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Jun 18, 2025

You don't even have to add it to "scripts/ci/pre-commit" -> you can easily run them directly from here.

@sunank200
Copy link
Copy Markdown
Collaborator Author

I would even propose to turn them into scripts and run them via pre-commit. Those tests are fast and can be easily run every time "task-sdk" changes and they are much more like "static" tests than "unit tests"

@potiuk These public‐api checks are true pytest tests - they import the package at runtime, inspect all, and assert what’s actually there. I think pre-commit hooks in Airflow are intended to be fast, static linters or formatters, and they shouldn’t be reaching into the code to execute it.

@sunank200 sunank200 force-pushed the move-task-sdk-docs-tests-docs branch 2 times, most recently from cbd066c to 4609e3c Compare June 18, 2025 09:18
@potiuk
Copy link
Copy Markdown
Member

potiuk commented Jun 18, 2025

Ah yeah - I see they run "sphinx-build"...

BTW. Why not making it as sphinx extension instead? We have also a number of sphinx extensions that are run during the build of documentation and it means that they are running as part of "doc build" phase automatically.

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Jun 18, 2025

@sunank200 sunank200 force-pushed the move-task-sdk-docs-tests-docs branch from 4609e3c to df6ebc2 Compare June 20, 2025 17:08
@sunank200 sunank200 force-pushed the move-task-sdk-docs-tests-docs branch from df6ebc2 to 0ae0f17 Compare June 23, 2025 05:21
@sunank200
Copy link
Copy Markdown
Collaborator Author

Ah yeah - I see they run "sphinx-build"...

BTW. Why not making it as sphinx extension instead? We have also a number of sphinx extensions that are run during the build of documentation and it means that they are running as part of "doc build" phase automatically.

@potiuk I can take as part of next PR.

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Jun 23, 2025

@potiuk I can take as part of next PR.

Yep. Not needed now :). Just something to consider.

@potiuk potiuk merged commit 8e15470 into apache:main Jun 23, 2025
71 checks passed
sunank200 added a commit to astronomer/airflow that referenced this pull request Jul 2, 2025
…he#51876)

(cherry picked from commit 8e15470)

Co-authored-by: Ankit Chaurasia <8670962+sunank200@users.noreply.github.com>
kaxil pushed a commit that referenced this pull request Jul 2, 2025
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