Skip to content

Conversation

@topherinternational
Copy link
Contributor

@topherinternational topherinternational commented Sep 4, 2024

Followup to #41967 and parallel to #41991

My proposal is that we remove the behavior that detected an --ignore switch and removed the pytest directory arg, and instead fail out of the test call when this scenario is detected.

Pytest prioritizes the test arg over the ignore switch, so this code was inverting the way pytest expects to be used which creates complexity when reasoning about breeze and the CI test suites. Also, with #41991, we should be not calling these kinds of scenarios anyway, and raising an error will help us locate any straggler cases.

IOW, the command pytest tests/providers/alpha --ignore=tests/providers/alpha will run the tests in tests/providers/alpha (the ignore is ignored). Our change in #40037 attempted to override this behavior and prioritize the ignore switch; it is simpler to understand the system if we detect these scenarios upstream instead of force-fitting the reverse of how pytest wants to work.

This change refactors the _run_test function in the following ways:

  • All arguments for docker down and docker run commands are built before any docker calls are made
  • A check is made to see if any pytest directory arg is also specified in an --ignore=<dir> switch; if so, a system exit is initiated
  • Some argument construction steps are moved into dedicated functions for readability and testability

Tests have been added to verify the exit functionality and also validate some of the collaborator calls. I didn't feel the need to test the entire control flow.


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

@topherinternational topherinternational force-pushed the refactor-run-tests branch 4 times, most recently from eacc8e9 to 2d7ebf1 Compare September 5, 2024 20:29
@topherinternational topherinternational changed the title Skip running a breeze test if exclusions remove all pytest directory args Raise SystemExit if breeze test specifies a directory that is also ignored Sep 5, 2024
@topherinternational topherinternational marked this pull request as ready for review September 5, 2024 23:48
@potiuk
Copy link
Member

potiuk commented Sep 10, 2024

I am at the Airflow Summit - not sure if I will be able to take a look before my holidays after :(

1 similar comment
@potiuk
Copy link
Member

potiuk commented Sep 10, 2024

I am at the Airflow Summit - not sure if I will be able to take a look before my holidays after :(

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Oct 30, 2024
@github-actions github-actions bot closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants