Skip to content

[Test] Reconcile TestProjectResolvers #124988

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Mar 21, 2025
Merged

[Test] Reconcile TestProjectResolvers #124988

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bfa4203
[Test] Single-project-only resolver should not support multi-project
ywangd Mar 17, 2025
1425f49
[Test] Reconcile TestProjectResolvers
ywangd Mar 17, 2025
854558e
fix
ywangd Mar 17, 2025
e3bf4af
Merge remote-tracking branch 'origin/main' into mp/test-resolver-sing…
ywangd Mar 20, 2025
aceb634
review feedback
ywangd Mar 20, 2025
a6db641
tweak
ywangd Mar 20, 2025
84368ce
Merge branch 'main' into mp/test-resolver-single-project-only
elasticmachine Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions test/framework/src/main/java/org/elasticsearch/cluster/project/TestProjectResolvers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.test.ESTestCase;

import java.util.Collection;
import java.util.Objects;
Expand Down Expand Up @@ -60,7 +61,7 @@ public boolean supportsMultipleProjects() {
* with the executeOnProject method. This is mostly useful in places where we just need a placeholder to satisfy
* the constructor signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think we should remove the last sentence. The "placeholder usage" is singleProjectOnly. mustExecuteFirst has a pretty niche use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's a leftover that should be removed.

*/
public static ProjectResolver singleProjectOnly() {
public static ProjectResolver mustExecuteFirst() {
Comment on lines -63 to +62
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return new ProjectResolver() {

private ProjectId enforceProjectId = null;
Expand Down Expand Up @@ -102,12 +103,25 @@ public boolean supportsMultipleProjects() {

/**
* This method returns a ProjectResolver that gives back the specified project-id when its getProjectId method is called.
* It also assumes it is the only project in the cluster state and throws if that is not the case.
* The ProjectResolver can work with cluster state containing multiple projects and its supportsMultipleProjects returns true.
*/
public static ProjectResolver singleProject(ProjectId projectId) {
return singleProject(projectId, false);
}

/**
* 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());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


public static ProjectResolver singleProjectOnly(ProjectId projectId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a bit of Javadoc explaining this method as well?

return singleProject(projectId, true);
}

private static ProjectResolver singleProject(ProjectId projectId, boolean only) {
Objects.requireNonNull(projectId);
return new ProjectResolver() {
Expand Down Expand Up @@ -144,7 +158,7 @@ public <E extends Exception> void executeOnProject(ProjectId otherProjectId, Che

@Override
public boolean supportsMultipleProjects() {
return true;
return only == false;
}
};
}
Expand Down
37 changes: 32 additions & 5 deletions .../framework/src/test/java/org/elasticsearch/cluster/project/TestProjectResolversTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
import org.elasticsearch.test.ESTestCase;

import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;

public class TestProjectResolversTests extends ESTestCase {
Expand All @@ -37,17 +39,42 @@ public void testAllProjects() {
public void testSingleProject() {
final ProjectId projectId = randomUniqueProjectId();
final ProjectResolver projectResolver = TestProjectResolvers.singleProject(projectId);
assertThat(projectResolver.supportsMultipleProjects(), is(true));
assertThat(projectResolver.getProjectId(), equalTo(projectId));

ClusterState state = buildClusterState(projectId, randomIntBetween(0, 10));
assertThat(projectResolver.getProjectMetadata(state), notNullValue());
}

public void testSingleProjectOnly_getProjectIdAndMetadata() {
public void testSingleProjectOnly() {
final ProjectResolver projectResolver = TestProjectResolvers.singleProjectOnly();
assertThat(projectResolver.supportsMultipleProjects(), is(false));
final var projectId = projectResolver.getProjectId();

ClusterState state = buildClusterState(projectId, 0);
assertThat(projectResolver.getProjectMetadata(state), notNullValue());

final IllegalStateException e = expectThrows(
IllegalStateException.class,
() -> projectResolver.getProjectMetadata(buildClusterState(projectId, randomIntBetween(1, 10)))
);
assertThat(e.getMessage(), containsString("Cluster has multiple projects"));
}

public void testDefaultProjectOnly() {
final ProjectResolver projectResolver = TestProjectResolvers.DEFAULT_PROJECT_ONLY;
assertThat(projectResolver.supportsMultipleProjects(), is(false));
assertThat(projectResolver.getProjectId(), equalTo(ProjectId.DEFAULT));

ClusterState state = buildClusterState(ProjectId.DEFAULT, 0);
assertThat(projectResolver.getProjectMetadata(state), notNullValue());
}

public void testMustExecuteFirst_getProjectIdAndMetadata() {
final ProjectId projectId = randomUniqueProjectId();
final ClusterState state = buildClusterState(projectId);

final ProjectResolver projectResolver = TestProjectResolvers.singleProjectOnly();
final ProjectResolver projectResolver = TestProjectResolvers.mustExecuteFirst();
expectThrows(UnsupportedOperationException.class, projectResolver::getProjectId);
expectThrows(UnsupportedOperationException.class, () -> projectResolver.getProjectMetadata(state));

Expand All @@ -57,9 +84,9 @@ public void testSingleProjectOnly_getProjectIdAndMetadata() {
});
}

public void testSingleProjectOnly_getProjectIds() {
public void testMustExecuteFirst_getProjectIds() {
{
final ProjectResolver projectResolver = TestProjectResolvers.singleProjectOnly();
final ProjectResolver projectResolver = TestProjectResolvers.mustExecuteFirst();
final ProjectId projectId = randomUniqueProjectId();
ClusterState state = buildClusterState(projectId);
assertThat(state.metadata().projects().values(), hasSize(1));
Expand All @@ -71,7 +98,7 @@ public void testSingleProjectOnly_getProjectIds() {
});
}
{
final ProjectResolver projectResolver = TestProjectResolvers.singleProjectOnly();
final ProjectResolver projectResolver = TestProjectResolvers.mustExecuteFirst();
final ProjectId projectId = randomUniqueProjectId();
ClusterState state = buildClusterState(projectId, randomIntBetween(1, 10));
assertThat(state.metadata().projects().values().size(), greaterThan(1));
Expand Down