Skip to content

Fix flaky test_connection_form_widgets timeout - #72122

Merged
potiuk merged 1 commit into
apache:mainfrom
aws-mwaa:ramitkataria/fix-flaky-test-connection-form-widgets
Aug 27, 2026
Merged

Fix flaky test_connection_form_widgets timeout#72122
potiuk merged 1 commit into
apache:mainfrom
aws-mwaa:ramitkataria/fix-flaky-test-connection-form-widgets

Conversation

@ramitkataria

@ramitkataria ramitkataria commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Every test in TestProviderManager resets the ProvidersManager singleton, but sys.modules stays warm across tests. The first test in the suite's order to trigger _import_info_from_all_hooks() therefore pays the one-time cost of cold-importing every provider hook module, and that test is test_connection_form_widgets. That cost sits right at the 60s default per-test execution timeout, so the test flakes on slower or loaded runners (observed roughly 1 in 6 runs of core-tests on 8-vCPU runners in a downstream CI). The sibling test_field_behaviours, which runs the same sweep against a warm module cache, takes about 3s.

Raising the marker to 120s matches the existing precedent in tests/unit/cli/commands/test_connection_command.py and test_task_command.py.


Was generative AI tooling used to co-author this PR?
  • Yes (Claude Fable 5)

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

Every test in TestProviderManager resets the ProvidersManager singleton,
but sys.modules stays warm across tests. The first test in the suite's
order to trigger _import_info_from_all_hooks() therefore pays the
one-time cost of cold-importing every provider hook module, and that
test is test_connection_form_widgets. That cost sits right at the 60s
default per-test execution timeout, so the test flakes on slower or
loaded runners (observed roughly 1 in 6 runs of core-tests on 8-vCPU
runners in a downstream CI). The sibling test_field_behaviours, which
runs the same sweep against a warm module cache, takes about 3s.

Raising the marker to 120s matches the existing precedent in
tests/unit/cli/commands/test_connection_command.py and
test_task_command.py.

@ColtenOuO ColtenOuO 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.

The motivation makes sense, but this seems like a mitigation tied to the current test collection order.

Both test_connection_form_widgets and test_field_behaviours can trigger _import_info_from_all_hooks(). If test_field_behaviours is run in isolation, or if the test order changes, it may pay the same cold-import cost while still using the default 60-second timeout.

Combining the two tests would associate the timeout with the complete operation, but it would also mix two otherwise distinct assertions. Therefore, if increasing the timeout is the intended solution, I think applying the same timeout marker to both tests would be more robust.

Also, if this test does hang, it will take twice as long before it gets killed.

I’d appreciate input from a committer or PMC member familiar with our CI setup on these considerations. The rest is just my two cents for your reference!

Thanks for the contribution!

@potiuk
potiuk merged commit a2ab8e8 into apache:main Aug 27, 2026
72 checks passed
@potiuk

potiuk commented Aug 27, 2026

Copy link
Copy Markdown
Member

Combining the two tests would associate the timeout with the complete operation, but it would also mix two otherwise distinct assertions. Therefore, if increasing the timeout is the intended solution, I think applying the same timeout marker to both tests would be more robust.

Feel free to improve it @ColtenOuO

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants