Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Apr 1, 2023

The --test-type and --test-types parameters were very similar, but they have enough difference to differentiate them even more:

The --test-type is specifically to run single test and it might include not only the regular "test-types" but also allow for cross-selection of tests from different other types (for example --test-type Postgres will run tests for Postgres database and they might come from Providers, Others, CLI etc.

Where --test-types was generally foreseen to be able to split the tests into "sepearated" groups that could be run in parallel.

The parameters have different defaults and even different choice of test type that you could choose from (and --test-types is a space-separated one to make it easier to pass it around in CI, where rather than passing multiple (variable number) of parameters, it's easier to pass a single, even space-separated list of tests to run.

This change is good to show the difference between then parameters and to stress that they are really quite different, also it makes it easier to avoid confusion people might have especially that the name was easy to have typo in.

In a way (but different than in the original issue it Fixes: #30407


^ 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
Copy link
Member Author

potiuk commented Apr 1, 2023

cc: @vandonr-amz - I made changes to breeze w/regards providers and I recalled the discussion from ##30407 and I found that it is really easy to just rename the --test-types parameter to avoid confusion . Here it is.

@vandonr-amz
Copy link
Contributor

renaming the parameter is a breaking change though. Won't it break the CI here in github ?

Also, I agree that the rename is a step in the right direction to reduce confusion, but I still think it's weird to have parameters that are very similar but slightly different and incompatible in their use :/

@potiuk potiuk changed the title Rebname --test-types to --parallel-test-types parameters Rename --test-types to --parallel-test-types parameters Apr 1, 2023
@potiuk potiuk force-pushed the rename-test-types branch 2 times, most recently from 299e65d to 210ca71 Compare April 1, 2023 21:58
@potiuk
Copy link
Member Author

potiuk commented Apr 1, 2023

I also corrected the Ci references. breeze is deliberately in Airflow repo so that they can evolve together and so we do not have to care about backwards compatibility. And this paran is mostly used in CI - most people even if run parallel tests they would run it with default value (all tests).

We have no expectations nor need about breeze being backwards compatible :)

I understand where you're coming from but i think it is really the matter of where you get 'common' part. We are reusing a lot of code under the hood to run the tests - including which test "type" is selected, and it's a bit arbitrary decision how to name parameters that internally might even consist of similar 'blocks' and use the same test types.

In this case i'd say separating it MORE is better because it removes confusion and typos

@potiuk
Copy link
Member Author

potiuk commented Apr 1, 2023

But of course if you would like to propose a PR changing it + i am happy to discuss seeing the code :)

@potiuk potiuk force-pushed the rename-test-types branch from 210ca71 to beaac2e Compare April 1, 2023 22:58
Copy link
Contributor

@vandonr-amz vandonr-amz left a comment

Choose a reason for hiding this comment

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

lgtm

@potiuk
Copy link
Member Author

potiuk commented Apr 2, 2023

Small quality of life improvement in breeze.

@potiuk potiuk force-pushed the rename-test-types branch from f84121a to 891f9f5 Compare April 4, 2023 08:21
The --test-type and --test-types parameters were very similar, but
they have enough difference to differentiate them even more:

The --test-type is specifically to run single test and it might
include not only the regular "test-types" but also allow for
cross-selection of tests from different other types (for example
--test-type Postgres will run tests for Postgres database and
they might come from Providers, Others, CLI etc.

Where --test-types was generally foreseen to be able to split
the tests into "sepearated" groups that could be run in
parallel.

The parameters have different defaults and even different choice
of test type that you could choose from (and --test-types is a
space-separated one to make it easier to pass it around in CI,
where rather than passing multiple (variable number) of parameters,
it's easier to pass a single, even space-separated list of tests
to run.

This change is good to show the difference between then parameters
and to stress that they are really quite different, also it makes
it easier to avoid confusion people might have especially that the
name was easy to have typo in.

In a way (but different than in the original issue it
Fixes: apache#30407
@potiuk potiuk force-pushed the rename-test-types branch from 891f9f5 to aadcce6 Compare April 4, 2023 09:56
@potiuk potiuk merged commit 2060643 into apache:main Apr 4, 2023
@potiuk potiuk deleted the rename-test-types branch April 4, 2023 11:30
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 4, 2023
We've added a new reference to test-types in apache#30450 and it clashed
with parameter rename in apache#30424. This resulted in bad merge
(not too dangerous, just causing missing optimisation in collection
elapsed time in case only a subset of test types were to be executed.
potiuk added a commit that referenced this pull request Apr 4, 2023
We've added a new reference to test-types in #30450 and it clashed
with parameter rename in #30424. This resulted in bad merge
(not too dangerous, just causing missing optimisation in collection
elapsed time in case only a subset of test types were to be executed.
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 5, 2023
The typo causes unnecessary delays on building regular PRs :(
It was introduced in apache#30424
potiuk added a commit that referenced this pull request Apr 5, 2023
The typo causes unnecessary delays on building regular PRs :(
It was introduced in #30424
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.

merge breeze's --test-type and --test-types options

3 participants