Skip to content

Conversation

@Dev-iL
Copy link
Collaborator

@Dev-iL Dev-iL commented Aug 3, 2025

As part of adding some debug logging in another PR, a lot of tests failed because they appear to capture all the logs then compare them (for equality) to some expected output.

The initial plan was to use caplog to isolate the logger of interest, then use membership/intersection for assertions instead of equality. However, upon closer inspection of the failures, it quickly became apparent that their majority was related to CLI tests, which assert against stdout and not some particular logger. So a different approach was taken, which is to create a context manager fixture that filters out messages originating from the logging module.

In addition, some uses of unittest.mock were replaced with pytest's mocker fixture.


^ 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.

Copilot AI review requested due to automatic review settings August 3, 2025 10:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a StdoutCaptureManager context manager to isolate stdout capture from logging output in CLI tests, preventing test failures caused by debug logging interference. The context manager filters out log messages that would otherwise contaminate stdout assertions in CLI command tests.

  • Adds a new StdoutCaptureManager class that removes stdout logging handlers during stdout capture
  • Replaces unittest.mock usage with pytest's mocker fixture across multiple test files
  • Updates CLI test files to use the new stdout_capture fixture instead of direct redirect_stdout calls

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

File Description
airflow-core/tests/unit/cli/conftest.py Adds the StdoutCaptureManager class and stdout_capture pytest fixture
airflow-core/tests/unit/cli/commands/test_*.py Updates CLI command tests to use new stdout capture fixture and pytest mocker

@Dev-iL Dev-iL force-pushed the Dev-iL/2508/stdout-capture branch 10 times, most recently from 85aa959 to b2607fc Compare August 7, 2025 14:34
Dev-iL and others added 4 commits August 7, 2025 18:58
+ Replace some uses of unittest.mock with pytest's  mocker fixture
`test_cli_connections_import_should_load_connections`: the test expects to see an error about "new3" but only sees "Imported connection new1". The test setup only creates "new0" and "new1", so "new3" doesn't exist. The test seems to have incorrect logic or expectations.
(theory: the test is looking at stdout, and if a warning is present there, it doesn't find the text it expects on the right line)
@Dev-iL Dev-iL force-pushed the Dev-iL/2508/stdout-capture branch from b2607fc to da8bbf2 Compare August 7, 2025 16:03
@potiuk potiuk merged commit 40b768a into apache:main Aug 7, 2025
191 checks passed
@potiuk
Copy link
Member

potiuk commented Aug 7, 2025

Let's hope there will be no more flakiness :)

@eladkal eladkal added this to the Airflow 3.1.0 milestone Aug 8, 2025
@Dev-iL Dev-iL deleted the Dev-iL/2508/stdout-capture branch August 8, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants