Skip to content

Comments

Selective checks for API did not match API test specification#28319

Merged
potiuk merged 1 commit intoapache:mainfrom
potiuk:fix-api-selection-for-selective-tests
Dec 12, 2022
Merged

Selective checks for API did not match API test specification#28319
potiuk merged 1 commit intoapache:mainfrom
potiuk:fix-api-selection-for-selective-tests

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Dec 12, 2022

The selective checks for the API had "^tests/api" as matching regexp, where we used "tests/api_connexion" to trigger the tests. This means that changes to "tests/api_internal" were not treated as "Other", because they were included in the API regexp.

This PR fixes it.


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

The selective checks for the API had "^tests/api" as matching regexp,
where we used "tests/api", "tests/api_connexion" to trigger the tests.
This means that changes to "tests/api_internal" were not treated as
"Other" as they were included in "^tests/api".

This PR fixes it.
@potiuk potiuk force-pushed the fix-api-selection-for-selective-tests branch from 197c1d3 to 2bce2d5 Compare December 12, 2022 22:03
@Taragolis
Copy link
Contributor

Taragolis commented Dec 12, 2022

Looks like the actual command is constructed in another place: https://github.com/apache/airflow/actions/runs/3680179020/jobs/6225576889#step:6:1901

Running tests tests/api tests/api_connexion 


Starting the tests with those pytest arguments: --verbosity=0 --strict-markers --durations=100 --maxfail=50 --color=yes --junitxml=/files/test_result-API-sqlite.xml --timeouts-order moi --setup-timeout=60 --execution-timeout=60 --teardown-timeout=60 --output=/files/warnings-API-sqlite.txt --disable-warnings -rfEX --with-db-init tests/api tests/api_connexion

@potiuk
Copy link
Member Author

potiuk commented Dec 12, 2022

Looks like the actual command is constructed in another place: https://github.com/apache/airflow/actions/runs/3680179020/jobs/6225576889#step:6:1901

Running tests tests/api tests/api_connexion 


Starting the tests with those pytest arguments: --verbosity=0 --strict-markers --durations=100 --maxfail=50 --color=yes --junitxml=/files/test_result-API-sqlite.xml --timeouts-order moi --setup-timeout=60 --execution-timeout=60 --teardown-timeout=60 --output=/files/warnings-API-sqlite.txt --disable-warnings -rfEX --with-db-init tests/api tests/api_connexion

Yep. This is exactly what we are synchronizing with. The test command specifically calls out the directories to use and the regexp in selective checks is used to determine if it should be run or not.

So what the change brings is syncing these two:

  1. selective tests determines which file changes should trigger the tests
  2. the command you pointed out executes those

The problem was that the regexp also matched test_api_internal because this is how regexp works when there is no '/'. This PR changes the regexp to take into account the fact that there might be other directores starting with tests_api - they were previously matched by regexp but not executed by the "API" test type (when you run pytest tests_api tests_connexion the tests_api_internal are not run.

When this PR is merged, if there is a change coming in tests_api_internal, it is treated as other which triggers "all" test. Which is what we want.

@potiuk potiuk merged commit 17131b3 into apache:main Dec 12, 2022
@potiuk potiuk deleted the fix-api-selection-for-selective-tests branch December 12, 2022 23:00
@Taragolis
Copy link
Contributor

Yeah that might be some kind of misunderstanding with test categorisation.
I assume initially that it should be a part of API tests but also it might be a part of Core.

And one question about Other right now in this category also added System (AIP-47) Tests, might be better move it out into the separate category?

@potiuk
Copy link
Member Author

potiuk commented Dec 13, 2022

And one question about Other right now in this category also added System (AIP-47) Tests, might be better move it out into the separate category?

System tests are specifically excluded - this is entire different "category" of tests and they are not automatically run in CI.
They should not show up in "Other" - but maybe they do actually in "selective checks" :). Maybe we should remove them :D.

@Taragolis
Copy link
Contributor

If we exclude tests/system of any category than we do not check is it parsable by pytest or not.
But command in Other really big right now:

  Running tests tests/triggers tests/timetables tests/testconfig tests/testconfig/conf tests/test_utils tests/test_utils/perf
tests/test_utils/perf/perf_kit tests/test_utils/perf/dags tests/test_utils/operators tests/task tests/task/task_runner
tests/system tests/system/utils tests/system/providers tests/system/providers/zendesk tests/system/providers/yandex
tests/system/providers/trino tests/system/providers/telegram tests/system/providers/tabular
tests/system/providers/tableau tests/system/providers/sqlite tests/system/providers/snowflake
tests/system/providers/slack tests/system/providers/singularity tests/system/providers/salesforce
tests/system/providers/qubole tests/system/providers/presto tests/system/providers/postgres
tests/system/providers/plexus tests/system/providers/papermill tests/system/providers/opsgenie
tests/system/providers/neo4j tests/system/providers/mysql tests/system/providers/microsoft
tests/system/providers/microsoft/winrm tests/system/providers/microsoft/mssql tests/system/providers/microsoft/azure
tests/system/providers/jenkins tests/system/providers/jdbc tests/system/providers/influxdb tests/system/providers/http
tests/system/providers/google tests/system/providers/google/workplace tests/system/providers/google/suite
tests/system/providers/google/suite/resources tests/system/providers/google/marketing_platform
tests/system/providers/google/leveldb tests/system/providers/google/firebase tests/system/providers/google/datacatalog
tests/system/providers/google/cloud tests/system/providers/google/cloud/workflows
tests/system/providers/google/cloud/vision tests/system/providers/google/cloud/video_intelligence
tests/system/providers/google/cloud/vertex_ai tests/system/providers/google/cloud/vertex_ai/resources
tests/system/providers/google/cloud/translate_speech tests/system/providers/google/cloud/translate
tests/system/providers/google/cloud/transfers tests/system/providers/google/cloud/transfers/resources
tests/system/providers/google/cloud/text_to_speech tests/system/providers/google/cloud/tasks
tests/system/providers/google/cloud/storage_transfer tests/system/providers/google/cloud/storage_transfer/resources
tests/system/providers/google/cloud/stackdriver tests/system/providers/google/cloud/sql_to_sheets
tests/system/providers/google/cloud/speech_to_text tests/system/providers/google/cloud/spanner
tests/system/providers/google/cloud/pubsub tests/system/providers/google/cloud/natural_language
tests/system/providers/google/cloud/ml_engine tests/system/providers/google/cloud/life_sciences
tests/system/providers/google/cloud/life_sciences/resources tests/system/providers/google/cloud/kubernetes_engine
tests/system/providers/google/cloud/gcs tests/system/providers/google/cloud/gcs/resources
tests/system/providers/google/cloud/datastore tests/system/providers/google/cloud/dataproc_metastore
tests/system/providers/google/cloud/dataproc tests/system/providers/google/cloud/dataproc/resources
tests/system/providers/google/cloud/dataprep tests/system/providers/google/cloud/dataplex
tests/system/providers/google/cloud/dataplex/resources tests/system/providers/google/cloud/dataform
tests/system/providers/google/cloud/dataflow tests/system/providers/google/cloud/dataflow/resources
tests/system/providers/google/cloud/data_loss_prevention
tests/system/providers/google/cloud/data_loss_prevention/resources tests/system/providers/google/cloud/compute
tests/system/providers/google/cloud/composer tests/system/providers/google/cloud/cloud_sql
tests/system/providers/google/cloud/cloud_memorystore tests/system/providers/google/cloud/cloud_functions
tests/system/providers/google/cloud/cloud_build tests/system/providers/google/cloud/cloud_build/resources
tests/system/providers/google/cloud/bigtable tests/system/providers/google/cloud/bigquery
tests/system/providers/google/cloud/bigquery/resources tests/system/providers/google/cloud/azure
tests/system/providers/google/cloud/automl tests/system/providers/google/ads tests/system/providers/github
tests/system/providers/ftp tests/system/providers/elasticsearch tests/system/providers/docker
tests/system/providers/dingding tests/system/providers/dbt tests/system/providers/dbt/cloud
tests/system/providers/databricks tests/system/providers/common tests/system/providers/common/sql
tests/system/providers/cncf tests/system/providers/cncf/kubernetes tests/system/providers/asana
tests/system/providers/apache tests/system/providers/apache/spark tests/system/providers/apache/pig
tests/system/providers/apache/livy tests/system/providers/apache/kylin tests/system/providers/apache/hive
tests/system/providers/apache/druid tests/system/providers/apache/drill tests/system/providers/apache/cassandra
tests/system/providers/apache/beam tests/system/providers/amazon tests/system/providers/amazon/aws
tests/system/providers/amazon/aws/utils tests/system/providers/alibaba tests/system/providers/airbyte tests/sensors
tests/security tests/secrets tests/plugins tests/operators tests/macros tests/listeners tests/lineage tests/kubernetes
tests/kubernetes/models tests/internal_api tests/hooks tests/decorators tests/datasets tests/dag_processing
tests/config_templates tests/cluster_policies tests/callbacks
  
  
  Starting the tests with those pytest arguments: --verbosity=0 --strict-markers --durations=100 --maxfail=50 --color=yes --junitxml=/files/test_result-Other-sqlite.xml --timeouts-order moi --setup-timeout=60 --execution-timeout=60 --teardown-timeout=60 --output=/files/warnings-Other-sqlite.txt --disable-warnings -rfEX --with-db-init tests/triggers tests/timetables tests/testconfig tests/testconfig/conf tests/test_utils tests/test_utils/perf tests/test_utils/perf/perf_kit tests/test_utils/perf/dags tests/test_utils/operators tests/task tests/task/task_runner tests/system tests/system/utils tests/system/providers tests/system/providers/zendesk tests/system/providers/yandex tests/system/providers/trino tests/system/providers/telegram tests/system/providers/tabular tests/system/providers/tableau tests/system/providers/sqlite tests/system/providers/snowflake tests/system/providers/slack tests/system/providers/singularity tests/system/providers/salesforce tests/system/providers/qubole tests/system/providers/presto tests/system/providers/postgres tests/system/providers/plexus tests/system/providers/papermill tests/system/providers/opsgenie tests/system/providers/neo4j tests/system/providers/mysql tests/system/providers/microsoft tests/system/providers/microsoft/winrm tests/system/providers/microsoft/mssql tests/system/providers/microsoft/azure tests/system/providers/jenkins tests/system/providers/jdbc tests/system/providers/influxdb tests/system/providers/http tests/system/providers/google tests/system/providers/google/workplace tests/system/providers/google/suite tests/system/providers/google/suite/resources tests/system/providers/google/marketing_platform tests/system/providers/google/leveldb tests/system/providers/google/firebase tests/system/providers/google/datacatalog tests/system/providers/google/cloud tests/system/providers/google/cloud/workflows tests/system/providers/google/cloud/vision tests/system/providers/google/cloud/video_intelligence tests/system/providers/google/cloud/vertex_ai tests/system/providers/google/cloud/vertex_ai/resources tests/system/providers/google/cloud/translate_speech tests/system/providers/google/cloud/translate tests/system/providers/google/cloud/transfers tests/system/providers/google/cloud/transfers/resources tests/system/providers/google/cloud/text_to_speech tests/system/providers/google/cloud/tasks tests/system/providers/google/cloud/storage_transfer tests/system/providers/google/cloud/storage_transfer/resources tests/system/providers/google/cloud/stackdriver tests/system/providers/google/cloud/sql_to_sheets tests/system/providers/google/cloud/speech_to_text tests/system/providers/google/cloud/spanner tests/system/providers/google/cloud/pubsub tests/system/providers/google/cloud/natural_language tests/system/providers/google/cloud/ml_engine tests/system/providers/google/cloud/life_sciences tests/system/providers/google/cloud/life_sciences/resources tests/system/providers/google/cloud/kubernetes_engine tests/system/providers/google/cloud/gcs tests/system/providers/google/cloud/gcs/resources tests/system/providers/google/cloud/datastore tests/system/providers/google/cloud/dataproc_metastore tests/system/providers/google/cloud/dataproc tests/system/providers/google/cloud/dataproc/resources tests/system/providers/google/cloud/dataprep tests/system/providers/google/cloud/dataplex tests/system/providers/google/cloud/dataplex/resources tests/system/providers/google/cloud/dataform tests/system/providers/google/cloud/dataflow tests/system/providers/google/cloud/dataflow/resources tests/system/providers/google/cloud/data_loss_prevention tests/system/providers/google/cloud/data_loss_prevention/resources tests/system/providers/google/cloud/compute tests/system/providers/google/cloud/composer tests/system/providers/google/cloud/cloud_sql tests/system/providers/google/cloud/cloud_memorystore tests/system/providers/google/cloud/cloud_functions tests/system/providers/google/cloud/cloud_build tests/system/providers/google/cloud/cloud_build/resources tests/system/providers/google/cloud/bigtable tests/system/providers/google/cloud/bigquery tests/system/providers/google/cloud/bigquery/resources tests/system/providers/google/cloud/azure tests/system/providers/google/cloud/automl tests/system/providers/google/ads tests/system/providers/github tests/system/providers/ftp tests/system/providers/elasticsearch tests/system/providers/docker tests/system/providers/dingding tests/system/providers/dbt tests/system/providers/dbt/cloud tests/system/providers/databricks tests/system/providers/common tests/system/providers/common/sql tests/system/providers/cncf tests/system/providers/cncf/kubernetes tests/system/providers/asana tests/system/providers/apache tests/system/providers/apache/spark tests/system/providers/apache/pig tests/system/providers/apache/livy tests/system/providers/apache/kylin tests/system/providers/apache/hive tests/system/providers/apache/druid tests/system/providers/apache/drill tests/system/providers/apache/cassandra tests/system/providers/apache/beam tests/system/providers/amazon tests/system/providers/amazon/aws tests/system/providers/amazon/aws/utils tests/system/providers/alibaba tests/system/providers/airbyte tests/sensors tests/security tests/secrets tests/plugins tests/operators tests/macros tests/listeners tests/lineage tests/kubernetes tests/kubernetes/models tests/internal_api tests/hooks tests/decorators tests/datasets tests/dag_processing tests/config_templates tests/cluster_policies tests/callbacks

@potiuk
Copy link
Member Author

potiuk commented Dec 13, 2022

We can exclude them from Other category when running tests actually.

potiuk added a commit to potiuk/airflow that referenced this pull request Dec 28, 2022
The "Other" test category automatically selects all tests that
are not included in any of the regular categories. That is to
make sure that we do not forget to add any directory that
has been added. However this led to a long directory selection
for "Other" category including system tests that have been
automatically added there. However those tests are always skipped
in regular tests and collecting those tests during "Other"
execution is not needed and slows it down.

Similarly System tests changes were treated as "Other change"
for incoming PRs. This means that any change to system tests
would trigger "all tests" rather than selective subset of
those - the same as any core change.

However System tests are also part of the documentation,
so any change in system tests should trigger docs
builds.

This change improves it in a few ways:

* Other tests now do not include "System Tests" - they are
  treated the same way as other test categories (but not
  included in test category selection for now - until we get
  a good way of breeze-integration for System Tests

* They are also excluded from treating them as "other" change
  when considering which tests to run. Changes to system tests
  will not trigger "all" tests, just those that accompanying
  changes would trigger.

* The changes to system tests only, however, triggers docs build
  because those are triggered by any source change.a

* The __pycache__ directories are removed from the list of
  "Other" packages to run.

* In order to make sure system tests are pytest-collectable,
  we perform pytest collection for all tests right after downloading
  the CI images and verifying them. This makes sure that the
  tests are collectible before we even attempt to run them - this way
  we avoid unnecessary machine spin-up and breze installation for the
  multiple jobs that run the tests. This slows down feedback time
  a litle, but should increase overall robustness of the test suite.

* an old, unused nosetest collection script doing the same in the past
  has been removed (it was discovered during implementation)

Noticed in: apache#28319
potiuk added a commit that referenced this pull request Dec 28, 2022
The "Other" test category automatically selects all tests that
are not included in any of the regular categories. That is to
make sure that we do not forget to add any directory that
has been added. However this led to a long directory selection
for "Other" category including system tests that have been
automatically added there. However those tests are always skipped
in regular tests and collecting those tests during "Other"
execution is not needed and slows it down.

Similarly System tests changes were treated as "Other change"
for incoming PRs. This means that any change to system tests
would trigger "all tests" rather than selective subset of
those - the same as any core change.

However System tests are also part of the documentation,
so any change in system tests should trigger docs
builds.

This change improves it in a few ways:

* Other tests now do not include "System Tests" - they are
  treated the same way as other test categories (but not
  included in test category selection for now - until we get
  a good way of breeze-integration for System Tests

* They are also excluded from treating them as "other" change
  when considering which tests to run. Changes to system tests
  will not trigger "all" tests, just those that accompanying
  changes would trigger.

* The changes to system tests only, however, triggers docs build
  because those are triggered by any source change.a

* The __pycache__ directories are removed from the list of
  "Other" packages to run.

* In order to make sure system tests are pytest-collectable,
  we perform pytest collection for all tests right after downloading
  the CI images and verifying them. This makes sure that the
  tests are collectible before we even attempt to run them - this way
  we avoid unnecessary machine spin-up and breze installation for the
  multiple jobs that run the tests. This slows down feedback time
  a litle, but should increase overall robustness of the test suite.

* an old, unused nosetest collection script doing the same in the past
  has been removed (it was discovered during implementation)

Noticed in: #28319
ephraimbuddy pushed a commit that referenced this pull request Jan 12, 2023
The selective checks for the API had "^tests/api" as matching regexp,
where we used "tests/api", "tests/api_connexion" to trigger the tests.
This means that changes to "tests/api_internal" were not treated as
"Other" as they were included in "^tests/api".

This PR fixes it.

(cherry picked from commit 17131b3)
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 13, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.5.1 milestone Jan 13, 2023
ephraimbuddy pushed a commit that referenced this pull request Mar 8, 2023
The "Other" test category automatically selects all tests that
are not included in any of the regular categories. That is to
make sure that we do not forget to add any directory that
has been added. However this led to a long directory selection
for "Other" category including system tests that have been
automatically added there. However those tests are always skipped
in regular tests and collecting those tests during "Other"
execution is not needed and slows it down.

Similarly System tests changes were treated as "Other change"
for incoming PRs. This means that any change to system tests
would trigger "all tests" rather than selective subset of
those - the same as any core change.

However System tests are also part of the documentation,
so any change in system tests should trigger docs
builds.

This change improves it in a few ways:

* Other tests now do not include "System Tests" - they are
  treated the same way as other test categories (but not
  included in test category selection for now - until we get
  a good way of breeze-integration for System Tests

* They are also excluded from treating them as "other" change
  when considering which tests to run. Changes to system tests
  will not trigger "all" tests, just those that accompanying
  changes would trigger.

* The changes to system tests only, however, triggers docs build
  because those are triggered by any source change.a

* The __pycache__ directories are removed from the list of
  "Other" packages to run.

* In order to make sure system tests are pytest-collectable,
  we perform pytest collection for all tests right after downloading
  the CI images and verifying them. This makes sure that the
  tests are collectible before we even attempt to run them - this way
  we avoid unnecessary machine spin-up and breze installation for the
  multiple jobs that run the tests. This slows down feedback time
  a litle, but should increase overall robustness of the test suite.

* an old, unused nosetest collection script doing the same in the past
  has been removed (it was discovered during implementation)

Noticed in: #28319

(cherry picked from commit e8657ce)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants