Refactor tests to check only relevant tasks#86351
Merged
gmarouli merged 5 commits intoelastic:masterfrom May 4, 2022
Merged
Conversation
Collaborator
|
Pinging @elastic/es-data-management (Team:Data Management) |
andreidan
approved these changes
May 3, 2022
Contributor
andreidan
left a comment
There was a problem hiding this comment.
LGTM, thanks Mary. Left a couple of minor suggestions
| */ | ||
| public static List<PersistentTasksCustomMetadata.PersistentTask<?>> findTasks(ClusterState clusterState, String taskName) { | ||
| PersistentTasksCustomMetadata tasks = clusterState.metadata().custom(PersistentTasksCustomMetadata.TYPE); | ||
| return tasks.tasks().stream().filter(t -> t.getTaskName().equals(taskName)).toList(); |
Contributor
There was a problem hiding this comment.
As this method is a special case of the other findTasks method, should we call the other one instead of reimplementing it here?
e.g.
findTasks(clusterState, Set.of(taskName))
Contributor
Author
There was a problem hiding this comment.
I wanted to avoid creating extra collections, but on second thought it's not worth the benefit. I will change it.
| * Retrieves the persistent tasks with the requested task names from the given cluster state. | ||
| */ | ||
| public static List<PersistentTasksCustomMetadata.PersistentTask<?>> findTasks(ClusterState clusterState, Set<String> taskNames) { | ||
| PersistentTasksCustomMetadata tasks = clusterState.metadata().custom(PersistentTasksCustomMetadata.TYPE); |
Contributor
There was a problem hiding this comment.
I believe this can be null? Should we check for null and return List.of() ?
Contributor
Author
There was a problem hiding this comment.
Oh, yes, I didn't think that it could be null. I will double check it.
Contributor
Author
|
@elasticmachine update branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With #86131 we introduce a persistent task that will be always running. This is interfering with the current way the tests included in this PR are written. These tests are expecting that the tasks related to the test are the only tasks running. This assumption will not hold soon. For this reason, we refactor the tests so that the assertions are handling only the tasks that are related to the test.
There is only one exception
BaseMlIntegTestCase, in this case it was easier to exclude unrelated tasks than to include all the possible tasks created as a side effect of the ML jobs. The constantUNRELATED_TASKSenabled developers to add tasks that need to be excluded, for example the new task we are introducing in #86131.