-
Notifications
You must be signed in to change notification settings - Fork 16.4k
[AIRFLOW-6720] Detect missing tests for providers package #7346
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
[AIRFLOW-6720] Detect missing tests for providers package #7346
Conversation
4b0141f to
4c302d9
Compare
Codecov Report
@@ Coverage Diff @@
## master #7346 +/- ##
==========================================
- Coverage 86.1% 85.86% -0.24%
==========================================
Files 871 871
Lines 40574 40574
==========================================
- Hits 34935 34839 -96
- Misses 5639 5735 +96
Continue to review full report at Codecov.
|
|
Nice. I think it's a good step. I'd like others to chime in - but it looks like an interesting approach. |
tests/test_project_structure.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think missing_tests_files <= MISSING_TEST_FILES is the proper condition, as missing_tests_files can be subset of MISSING_TEST_FILES. In case someone added a missing test this test should pass and test_keep_missing_test_files_update should fail. Otherwise, we are testing the same thing two times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good with equals. The goal is - similar as with pylint_todo.txt to go with this list to empty. So when someone adds test file, the test will fail and it will be the right time for the person who adds this file to remove it from the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but we have two tests. One that checks missing files (this one) and second one that checks if someone adjusted the MISSING_TEST_FILES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks you. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. that's nicer now!
4c302d9 to
53845c6
Compare
|
Noted on the cc - picked up a couple of these over in #7468. |
During the introduction of AIP-21, I corrected the test file names. Very often the file name in the application did not match the test file name. Now I would like to prevent regression, so I add tests that detect typos in names. This will also force you to add tests.
This is an experiment that we can extend to other packages in the project in the future.
CC: @coopergillan
Issue link: AIRFLOW-6720
Make sure to mark the boxes below before creating PR: [x]
[AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID** For document-only changes commit message can start with
[AIRFLOW-XXXX].In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.