Skip to content

Conversation

@frankcash
Copy link
Contributor

@frankcash frankcash commented Jan 6, 2023

will resolve #28768

Adds regexp check for @pytest.mark.xfail in files ^tests/.*\.py$

Screen Shot 2023-01-06 at 3 23 24 PM


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

@eladkal
Copy link
Contributor

eladkal commented Jan 6, 2023

We still have 1 test that needs to be excluded #23539 as we didnt finish removing xfail everywhere

@eladkal eladkal requested a review from uranusjr January 6, 2023 19:21
@frankcash
Copy link
Contributor Author

@eladkal we could also wait to merge this until #28724 is merged

@Taragolis
Copy link
Contributor

We also have one xfail marked test which not listed in #23539

@pytest.mark.xfail(reason="We did not reach full coverage yet")
def test_missing_assets(self):
super().test_missing_assets()

One of suggestion is remove this kind of tests, see: #28459 (comment)

@eladkal
Copy link
Contributor

eladkal commented Jan 9, 2023

@frankcash I suggest to add exclusion for the 1-2 remaining tests
cleaning it after tests are refactored is simple

@frankcash
Copy link
Contributor Author

frankcash commented Jan 9, 2023

@eladkal does this look like adding those files specifically to the exclude file list? exclude: ^airflow/|^docs/. or extend the regex?

@frankcash
Copy link
Contributor Author

@eladkal

@frankcash frankcash requested review from eladkal and removed request for ashb, jedcunningham, potiuk and uranusjr January 10, 2023 13:55
@ashb
Copy link
Member

ashb commented Jan 10, 2023

This is a -0.99 from me. XFail exists for a reason, we can discourage it's use without needing to remove it.

See #28840 for where I think an xfail makes sense.

@potiuk
Copy link
Member

potiuk commented Jan 10, 2023

This is a -0.99 from me. XFail exists for a reason, we can discourage it's use without needing to remove it.

Yep. I think there are reasons why you might want to have xfail. Some of the other fixes xfails in #23539 were simply mis-used - their reason was "the tests were flaky" but using xfail for the reason of "we know it should fail and we can fix it later" is quite ok.

@potiuk potiuk closed this Jan 10, 2023
@potiuk
Copy link
Member

potiuk commented Jan 10, 2023

BTW. it does not mean that the encouragement of getting rid of those xfals is bad idea (if done correctly).

@ashb
Copy link
Member

ashb commented Jan 11, 2023

Oh yeah I'm cool with "don't use XFail for flaky tests" etc.

@frankcash frankcash deleted the frankcash/28768 branch January 11, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pre-commit to prevent using xfail tests

6 participants