Skip to content
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 selective tests checks when system tests are modified #45131

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Dec 21, 2024

When providers system tests were modified in provider tests were modified, selective checks had two bugs that compounded led to provider tests being skipped.

  1. Modification of system tests lead to "all providers affected"
    condition (wrongly) - because "TESTS" were checked as root path
    before "SYSTEM TESTS" - and since system tests were subfolder
    of tests, the code that checked if system tests path belongs
    to provider never found it.

  2. "All Providers affected" in such case (when it was not caused by
    another condition lead to "skip-provider-tests" set to true due
    to missing "else" in one of the conditions.

This PR fixes both cases and simplifies the conditions so that they are easier to understand and modify. Those conditions will be further simplified when we separate providers to separate projects #44511 because there we will not have to check separately several sub-folders of airflow, but for now it's good enough.

Some of those conditions are simplified as well because we are in Python 3.9+ and "is_relative_to" is now available in Path.


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

Copy link
Member

@gopidesupavan gopidesupavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good fix, thanks Jarek.

It seems there are might be some pr's until now went out without providers tests. and good thing we have schedule run to spot out these like the one recent :)

@potiuk potiuk force-pushed the fix-selective-tests-edge-case-on-system-tests branch from be4a4e2 to 41102e0 Compare December 21, 2024 09:19
@potiuk
Copy link
Member Author

potiuk commented Dec 21, 2024

It seems there are might be some pr's until now went out without providers tests. and good thing we have schedule run to spot out these like the one recent :)

Well. I think it was quite rare - It's only when system tests were modified and NO provider.yaml was modified - that would trigger another path I think, and this is a rare case where system tests are modified together with regular tests and code. When you add new classes with system tests you also usually modify the provider.yaml to add those hooks/operators you added.

But yes. "canary" builds are necessary and they are running all tests by-design to catch such cases. That was an important and deliberate design decision.

When providers system tests were modified in provider tests were
modified, selective checks had two bugs that compounded led to provider tests
being skipped.

1) Modification of system tests lead to "all providers affected"
   condition (wrongly) - because "TESTS" were checked as root path
   before "SYSTEM TESTS" - and since system tests were subfolder
   of tests, the code that checked if system tests path belongs
   to provider never found it.

2) "All Providers affected" in such case (when it was not caused by
   another condition lead to "skip-provider-tests" set to true due
   to missing "else" in one of the conditions.

This PR fixes both cases and simplifies the conditions so that they are
easier to understand and modify. Those conditions will be further
simplified when we separate providers to separate projects apache#44511
because there we will not have to check separately several sub-folders
of airflow, but for now it's good enough.

Some of those conditions are simplified as well because we are in
Python 3.9+ and "is_relative_to" is now available in Path.
@potiuk potiuk force-pushed the fix-selective-tests-edge-case-on-system-tests branch from 41102e0 to 78b5682 Compare December 21, 2024 10:38
@potiuk potiuk merged commit 31af3a5 into apache:main Dec 21, 2024
96 checks passed
@potiuk potiuk deleted the fix-selective-tests-edge-case-on-system-tests branch December 21, 2024 15:09
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.

3 participants