Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Apr 7, 2023

While adding a feature flag for AIP-44 I noticed that due to a weird naming we have in tests, the "api_internal" tests were actually excluded from running - this was due to a combination of factors:

  • When API tests are are run, only "api" and "api_connexion" were added to API_tests
  • We already have "api" folder in "tests" (for experimental api)
  • finding "Other" tests should cover it but it exluded "api" tests but the way it is implemented, it took the "api" prefix and excluded all the test directories that were starting with "api" (including "api_internal/*" ones

This change addresses it twofold:

  • The "api_internal" tests are added explicitly to the "API" test type
  • The "tests/api" folder with tests for the experimental API has been renamed to "api_experimental" (including integration tests)

This should set the "internal_api" tests to run in the API test type, and renaming the "api" to api_experimental should avoid accidental skipping the tests in case someone adds "tests/api_SOMETHING" in the future.


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

@potiuk potiuk requested a review from ashb as a code owner April 7, 2023 07:38
@potiuk potiuk requested a review from eladkal April 7, 2023 07:41
@potiuk potiuk force-pushed the enable-tests-for-api-internal branch from 934b046 to b2ea777 Compare April 7, 2023 10:04
@potiuk potiuk requested a review from pierrejeambrun April 7, 2023 10:18
@potiuk
Copy link
Member Author

potiuk commented Apr 7, 2023

I realized the internal_api tests were all skipped after the feature flag change (and the reason was that the env vars from ci.yml were not passed to inside docker-compose) - I extracted that change from this PR to #30521

While adding a feature flag for AIP-44 I noticed that due to a
weird naming we have in tests, the "api_internal" tests were
actually excluded from running - this was due to a combination of
factors:

* When API tests are are run, only "api" and "api_connexion" were
  added to API_tests
* We already have "api" folder in "tests" (for experimental api)
* finding "Other" tests should cover it but it exluded "api" tests but
  the way it is implemented, it took the "api" prefix and excluded
  all the test directories that were starting with "api" (including
  "api_internal/*" ones

This change addresses it twofold:

* The "api_internal" tests are added explicitly to the "API"
  test type
* The "tests/api" folder with tests for the experimental API
  has been renamed to "api_experimental" (including integration
  tests)

This should set the "internal_api" tests to run in the API test
type, and renaming the "api" to api_experimental should avoid
accidental skipping the tests in case someone adds
"tests/api_SOMETHING" in the future.
@potiuk potiuk force-pushed the enable-tests-for-api-internal branch from b2ea777 to 73c2b86 Compare April 7, 2023 15:49
else
CLI_TESTS=("tests/cli")
API_TESTS=("tests/api" "tests/api_connexion")
API_TESTS=("tests/api_experimental" "tests/api_connexion" "tests/api_internal/")
Copy link
Member

Choose a reason for hiding this comment

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

Also here

potiuk and others added 2 commits April 7, 2023 20:15
@potiuk potiuk merged commit 797246a into apache:main Apr 7, 2023
@potiuk potiuk deleted the enable-tests-for-api-internal branch April 7, 2023 18:51
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.

2 participants