Skip to content

Add prek hook to ban caplog usage in test files#65421

Open
amoghrajesh wants to merge 1 commit intoapache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Open

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 1 commit intoapache:mainfrom
amoghrajesh:worktree-caplog-prek-hook

Conversation

@amoghrajesh
Copy link
Copy Markdown
Contributor


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Noticed recently in #65232 that caplog was used.

Wanted to add a prek hook to now allow this as per our coding standards: https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards.

Intent is to enforce the testing standard that tests should check logic rather than log output.

I am proposing to solve this by adding a pygrep hook (check-no-caplog-in-tests) to airflow-core, providers, and task-sdk pre commit config. The hook matches \bcaplog\b in test files and fails if any new usage is introduced.

Existing usages (35 core, 89 provider, 6 task-sdk files) are unaffected — prek only runs on files changed in a given PR, so cleanup can happen incrementally.


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@jason810496
Copy link
Copy Markdown
Member

Nice improvement, I agree that we should to add the prek hook to prevent more caplog usage in the first place.

Copy link
Copy Markdown
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

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

Nice, thanks for including these, Amogh! I think we were checking in the root beforehand

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk potiuk force-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5 Compare April 17, 2026 15:14
@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 17, 2026

Rebased to run with "full tests"

@bugraoz93
Copy link
Copy Markdown
Contributor

bugraoz93 commented Apr 17, 2026

We should use the label for this now as it fails for all and maybe for accepting some if needed

@bugraoz93
Copy link
Copy Markdown
Contributor

bugraoz93 commented Apr 17, 2026

Since we call directly pygrep maybe we need to add skip option to prek but I don't know how or even possible

Copy link
Copy Markdown
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Cool!

But probably you need to add an exclusion list for all cases still in the repo or are we (still) clean?

@jscheffl
Copy link
Copy Markdown
Contributor

Cool!

But probably you need to add an exclusion list for all cases still in the repo or are we (still) clean?

Oh, just see the list of static checks, seems like a call for helping hands is needed!

@amoghrajesh
Copy link
Copy Markdown
Contributor Author

Oh haha, seems I need to do that. Let me take a stab at that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools area:providers area:task-sdk backport-to-v3-2-test Mark PR with this label to backport to v3-2-test branch full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants