Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Add prek hook to ban caplog usage in test files by amoghrajesh · Pull Request #65421 · apache/airflow · GitHub
Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add prek hook to ban caplog usage in test files by amoghrajesh · Pull Request #65421 · apache/airflow · GitHub
Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add prek hook to ban caplog usage in test files by amoghrajesh · Pull Request #65421 · apache/airflow · GitHub
Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Add prek hook to ban caplog usage in test files by amoghrajesh · Pull Request #65421 · apache/airflow · GitHub
Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add prek hook to ban caplog usage in test files by amoghrajesh · Pull Request #65421 · apache/airflow · GitHub
Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add prek hook to ban caplog usage in test files by amoghrajesh · Pull Request #65421 · apache/airflow · GitHub
Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Add prek hook to ban caplog usage in test files by amoghrajesh · Pull Request #65421 · apache/airflow · GitHub
Skip to content

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

Closed
amoghrajesh wants to merge 4 commits into
apache:mainfrom
amoghrajesh:worktree-caplog-prek-hook
Closed

Add prek hook to ban caplog usage in test files#65421
amoghrajesh wants to merge 4 commits into
apache: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.

@bugraoz93bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

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

@potiukpotiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 17, 2026
@potiuk
potiukforce-pushed the worktree-caplog-prek-hook branch from da67f8a to 282aca5CompareApril 17, 2026 15:14
@potiuk

Copy link
Copy Markdown
Member

Rebased to run with "full tests"

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@bugraoz93

bugraoz93 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

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

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

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
ContributorAuthor

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

@Lee-WLee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought we're to use ast 🤔 but pygrep should be good enough

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be better to follow the convention of

by using a .txt to track the current violations. Then we could defer the fix for replacing all the caplog usage (or make it as good first issue contribution opportunity for the community)

Based on the CI error logs, there're hundreds violations now, refactoring in single PR might cause even bigger test regression IMHO.

@amoghrajeshamoghrajesh self-assigned this May 25, 2026
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Yes, thanks @jason810496. I will get to this next week when I have some time.

@amoghrajesh
amoghrajesh marked this pull request as draft May 27, 2026 04:28
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Finally got back to this one.

@amoghrajesh
amoghrajesh marked this pull request as ready for review July 10, 2026 06:44

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after moving the generated known usage, thanks!

Comment threadgenerated/known_caplog_usage.txt

@ashbashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1. Now we are using strictlog the caplog fixture is better than mocking.

@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Closing in favour of: #69803

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

Labels

area:dev-toolsarea:providersarea:task-sdkfull tests neededWe 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.

8 participants

@amoghrajesh@jason810496@potiuk@bugraoz93@jscheffl@ashb@Lee-W@sunank200