Repository navigation
Add unit tests for the FAB app-init extensions - #72986
Closed
ShousenZHANG wants to merge 1 commit into
Closed
ShousenZHANG wants to merge 1 commit into
ShousenZHANG wants to merge 1 commit into
Conversation
Five of the seven `providers/fab/.../www/extensions/` modules were listed in `OVERLOOKED_TESTS` with no tests at all, and `create_app()` calls all five while building the Flask app, so a regression in any of them either stops the app from starting or silently changes what it serves. What the tests pin, per module: - `init_wsgi_middlewares`: the five `[fab] proxy_fix_x_*` options are set to five distinct values, so a mix-up between the five near-identical `ProxyFix` keyword arguments fails; and `wsgi_app` is left untouched when the option is off. - `init_security`: the comma split and per-entry strip, the import order, the default backend, and `ImportError` becoming `AirflowException`. An empty `auth_backends` raises `ValueError` instead, which `except ImportError` does not catch - recorded as current behaviour. - `init_jinja_globals`: the three `default_timezone` branches including the non-callable guard, hostname redaction when `[fab] expose_hostname` is off, the four navbar colours, and the two opposite globals driven by `enable_plugins`. - `init_manifest_files`: the `dist/` prefixing, the fallback to the raw filename when the manifest is missing or the key is unknown, and the debug-mode re-read. - `init_views`: `view` not being forwarded twice for named views, `add_view_no_menu` for unnamed ones, the named-view-without-a-view log, blueprint registration, and the 500/404 handler pair. Verified by mutation rather than by assertion count: 22 one-line breakages of the five modules were each applied in turn, and all 22 make the new tests fail. Drops the five matching paths from `OVERLOOKED_TESTS`, leaving `init_appbuilder` (615 lines, its own PR) and `init_session` (already covered by #72825). Generated-by: Claude Code
ShousenZHANG
force-pushed
the
fab-ext-tests
branch
from
September 11, 2026 17:28
ac301bf to
358731b
Compare
Contributor
|
The issue has been closed, no need to add all these tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five of the seven
providers/fab/src/airflow/providers/fab/www/extensions/modules are listed inOVERLOOKED_TESTSwith no tests at all, andcreate_app()reaches all five while building the Flask app:configure_manifest_filesatwww/app.py:113andinit_api_authat:114, theninit_error_handlersat:118,init_pluginsat:127(only whenenable_plugins),init_jinja_globalsat:130andinit_wsgi_middlewareat:131inside theapp_contextopened at:116. Nothing increate_appcatches, so a failure in any of them stops the app from starting; a silent behaviour change ininit_wsgi_middlewareorinit_jinja_globalsinstead alters what gets served.Grouped into one PR per the request on #35442 — "Batch a provider's worth of missing test modules into a single PR instead."
What the tests pin
init_wsgi_middlewares[fab] proxy_fix_x_*options set to five distinct values, so a mix-up between the five near-identicalProxyFixkeyword arguments fails;wsgi_appuntouched when the option is offinit_securityImportError→AirflowExceptioninit_jinja_globalsdefault_timezonebranches incl. the non-callable guard, hostname redaction when[fab] expose_hostnameis off, the four navbar colours, and the two opposite globals driven byenable_pluginsinit_manifest_filesdist/prefixing, the fallback to the raw filename when the manifest is missing or the key unknown, the debug-mode re-readinit_viewsviewnot forwarded twice for named views,add_view_no_menufor unnamed ones, the named-view-without-a-view log, blueprint registration, the 500/404 handler pairOne behaviour is recorded rather than asserted as correct: an empty
[fab] auth_backendsmakesinit_api_authcallimport_module(""), which raisesValueError, and the function'sexcept ImportErrordoes not catch it — so it escapes as a bareValueErrorinstead of theAirflowExceptionevery other bad backend produces. The test documents that; fixing it belongs in a change to the module, not here.Mutation results
Assertion count says nothing about whether a test can fail, so each of the five modules was broken one line at a time. 22 of 22 mutations are caught:
init_wsgi_middlewares(4/4) —x_hostreadingPROXY_FIX_X_PORT; dropping thex_prefix=keyword; inverting theENABLE_PROXY_FIXguard; hardcodingx_for=1init_security(4/4) — dropping.strip(); swallowing theImportError; not appending toapp.api_auth; wideningexcept ImportErrortoexcept Exceptioninit_jinja_globals(6/6) — not upper-casingutc; dropping the non-callablelocal_timezoneguard; inverting the hostname redaction; swapping two navbar colours; un-invertingdisable_nav_bar; letting the version lookup raiseinit_manifest_files(4/4) — dropping thedist/prefix; narrowing the bareexcept; removing the debug re-read; ignoring the manifest lookupinit_views(4/4) — forwarding the whole view dict toadd_view; usingadd_viewfor unnamed views; swapping the 500/404 handlers; skipping blueprint registrationScope and conflicts
This clears 5 of the 7
fab/www/extensionsexemptions. The two left areinit_appbuilder(615 lines, needs a realAirflowAppBuilder) andinit_session, which #72825 already covers.Two things a reviewer should know:
providers/fab/tests/unit/fab/www/extensions/__init__.py— the directory does not exist onmain, so whichever lands second gets an add/add conflict. Happy to rebase onto Add FAB option to log users out after a maximum session lifetime #72825; say the word and I will.OVERLOOKED_TESTSremoval is currently unenforced.test_project_structure.py:227doescurrent_test_files.intersection(OVERLOOKED_TESTS), comparing aset[Path]against alist[str], so that assertion can never fire. I removed the five lines because the list should reflect reality, not because CI would otherwise fail. Reject stale provider test exemptions #71985 already fixes that comparison and prunes stale entries, so I have not touched it.init_plugins' pre-3.1/pre-3.2 branches are unreachable onmain(AIRFLOW_V_3_1_PLUSandAIRFLOW_V_3_2_PLUSare both true) and are not covered;TestInitPluginscarries a matchingskipif.Verification
pytest providers/fab/tests/unit/fab/www/extensions/— 27 passedpytest airflow-core/tests/unit/always/test_project_structure.py— 10 passed, 1 pre-existing xfailruff checkandruff format --check— cleanprek run --files <the 8 changed files>— 0 failedWas generative AI tooling used to co-author this PR?
Generated-by: Claude Code following the guidelines
The tests were written by an AI assistant. I checked the call sites in
www/app.py, ran the 22 mutations above to confirm every test can actually fail, and ran the commands under Verification.