[Test] Reconcile TestProjectResolvers #124988
Conversation
In MP-1749, we differentiate between single-project and single-project only resolvers. The later should not support multi-project.
This PR updates the different methods in TestProjectResolvers so that their names are more accurate and behaviours to be more as expected.
ywangd
added
>test
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| public static ProjectResolver singleProjectOnly() { | ||
| public static ProjectResolver mustExecuteFirst() { |
There was a problem hiding this comment.
This method is used in quite a few places in tests. Majority of them (if not all) really just need a projectResolver as a placehoder. The choice of using this variant is probably based on the method name and has no need for its execute behaviour. I renamed this method and added a new singleProjectOnly which does only what the name suggests. Hopefully this should satisfy all existing usages. But if not, we can tweak the places where it needs more.
| /** | ||
| * This method returns a ProjectResolver that returns a random unique project-id. | ||
| * It also assumes it is the only project in the cluster state and throws if that is not the case. | ||
| * In addition, the ProjectResolvers returns false for supportsMultipleProjects. | ||
| */ | ||
| public static ProjectResolver singleProjectOnly() { | ||
| return singleProjectOnly(ESTestCase.randomUniqueProjectId()); | ||
| } |
There was a problem hiding this comment.
I'm not sure I get the goal of the implementation of this method. I understand that this method is mostly used as a placeholder, so we don't actually care about the project ID. However, by supplying a random project ID and checking if it exists in the cluster state, don't we always just return an exception if any of the methods are called? Wouldn't it make more sense to create a dedicated project resolver instance that just throws on every use? That would make the intent of this method/project resolver more clear.
There was a problem hiding this comment.
It does work for getProjectId(), supportsMultipleProjects(), executeOnProject(...) etc . I am not sure whether any of the existing usages rely on any of these. Probably not, I need to check.
| return singleProjectOnly(ESTestCase.randomUniqueProjectId()); | ||
| } | ||
|
|
||
| public static ProjectResolver singleProjectOnly(ProjectId projectId) { |
There was a problem hiding this comment.
Can we add a bit of Javadoc explaining this method as well?
| * with the executeOnProject method. This is mostly useful in places where we just need a placeholder to satisfy | ||
| * the constructor signature. |
There was a problem hiding this comment.
Nit: I think we should remove the last sentence. The "placeholder usage" is singleProjectOnly. mustExecuteFirst has a pretty niche use case.
There was a problem hiding this comment.
Yeah it's a leftover that should be removed.
|
@elasticmachine update branch |
This PR updates the different methods in TestProjectResolvers so that their names are more accurate and behaviours to be more as expected. For example, In MP-1749, we differentiate between single-project and single-project only resolvers. The later should not support multi-project.
This PR updates the different methods in TestProjectResolvers so that their names are more accurate and behaviours to be more as expected. For example, In MP-1749, we differentiate between single-project and single-project only resolvers. The later should not support multi-project.
This PR updates the different methods in TestProjectResolvers so that their names are more accurate and behaviours to be more as expected. For example, In MP-1749, we differentiate between single-project and single-project only resolvers. The later should not support multi-project.
4 participants
This PR updates the different methods in TestProjectResolvers so that
their names are more accurate and behaviours to be more as expected.
For example, In MP-1749, we differentiate between single-project and
single-project only resolvers. The later should not support multi-project.