-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix heisentest by ignoring the basepath #11993
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
|
@potiuk @j-y-matsubara does this hypothesis sound plausible to you? |
Great findings! Indeed that looks like a plausible explanation AND it actually fixes a real problem that actual user might not have realized! Thanks @megaserg ! |
Awesome work, congrats on your first merged pull request! |
The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it! |
I disagree with you bot :) |
This is a follow-up after apache#11993 - the behaviour of airflowignore changes significantly enough to require a warning.
This is a follow-up after #11993 - the behaviour of airflowignore changes significantly enough to require a warning.
Problem:
The issue #10988 suspects that there's a dependency on side-effects from other tests.
However, the test itself seems pretty hermetic, creating a new randomly-named temp directory every time.
My guess is that there is a case where the randomly-generated name contains one of the ignored tokens.
For example, probability of a random 8-character name containing
"not"
is roughly6 / 37^3
~ 0.01%.In this case, all files will be wrongfully ignored because we use the absolute path to perform the pattern matching.
Solution:
Use the relative path to perform the pattern matching. Also, look for the first match only (no need for
re.findall()
).Validation:
The modified test intentionally includes ignored token
"not"
in the name of the base path. With this modification, the test fails 100% of the time, unless the suggested fix infind_path_from_directory()
is applied.Closes: #10988