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

Auto configure all test tasks #34666

Merged
merged 6 commits into from
Oct 24, 2018

Conversation

alpar-t
Copy link
Contributor

@alpar-t alpar-t commented Oct 20, 2018

With this change, we apply the common test config automatically to all
newly created test tasks instead of opting in specifically.

For plugin authors using the plugin externally this means that the
configuration will be applied to their RandomizedTestingTasks as well.

The purpose of the task is to simplify setup and make it easier to
change projects that use the test task but actually run integration
tests to use a task called integTest for clarity, but also because
we may want to configure and run them differently.
E.x. using different levels of concurrency.

With this change, we apply the common test config automatically to all
newly created tasks instead of opting in specifically.

For plugin authors using the plugin externally this means that the
configuration will be applied to their RandomizedTestingTasks as well.

The purpose of the task is to simplify setup and make it easier to
change projects that use the `test` task but actually run integration
tests to use a task called `integTest` for clarity, but also because
we may want to configure and run them differently.
E.x. using different levels of concurrency.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM. One minor naming ask.

/** Returns a closure of common configuration shared by unit and integration tests. */
static Closure commonTestConfig(Project project) {
return {
static void applyCommonTestConfigPresentAndFuture(Project project) {
Copy link
Member

Choose a reason for hiding this comment

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

We have other methods here which apply to all existing and future items of a type. I don't think we need such a verbose name for these cases. configureTests should be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In retrospect that does seem excessive.
I think there is some value in coming up with something other than apply and configure to distinguish between what configures the current state vs attaching configuration to the domain object containers that affect future state.
For me this is one of the top confusions and sources of bugs, but we can do this in a follow up, I simplified the name for now.

@alpar-t
Copy link
Contributor Author

alpar-t commented Oct 23, 2018

@elasticmachine test this please

@alpar-t alpar-t merged commit 795d57b into elastic:master Oct 24, 2018
@alpar-t alpar-t deleted the auto-configure-all-test-tasks branch October 24, 2018 13:05
alpar-t added a commit to alpar-t/elasticsearch that referenced this pull request Oct 24, 2018
With this change, we apply the common test config automatically to all
newly created tasks instead of opting in specifically.

For plugin authors using the plugin externally this means that the
configuration will be applied to their RandomizedTestingTasks as well.

The purpose of the task is to simplify setup and make it easier to
change projects that use the `test` task but actually run integration
tests to use a task called `integTest` for clarity, but also because
we may want to configure and run them differently.
E.x. using different levels of concurrency.
@alpar-t alpar-t mentioned this pull request Oct 24, 2018
alpar-t added a commit that referenced this pull request Oct 25, 2018
* Auto configure all test tasks (#34666)

With this change, we apply the common test config automatically to all
newly created tasks instead of opting in specifically.

For plugin authors using the plugin externally this means that the
configuration will be applied to their RandomizedTestingTasks as well.

The purpose of the task is to simplify setup and make it easier to
change projects that use the `test` task but actually run integration
tests to use a task called `integTest` for clarity, but also because
we may want to configure and run them differently.
E.x. using different levels of concurrency.

* Convert tests using old method
kcm pushed a commit that referenced this pull request Oct 30, 2018
With this change, we apply the common test config automatically to all
newly created tasks instead of opting in specifically.

For plugin authors using the plugin externally this means that the
configuration will be applied to their RandomizedTestingTasks as well.

The purpose of the task is to simplify setup and make it easier to
change projects that use the `test` task but actually run integration
tests to use a task called `integTest` for clarity, but also because
we may want to configure and run them differently.
E.x. using different levels of concurrency.
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >non-issue Team:Delivery Meta label for Delivery team v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants