Skip to content

test: simplify tests #290

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 1 commit into from
Apr 23, 2025
Merged

test: simplify tests #290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions config-service-impl/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ dependencies {
compileOnly(commonLibs.lombok)

testImplementation(commonLibs.junit.jupiter)
testImplementation(commonLibs.mockito.junit)
testImplementation(commonLibs.mockito.core)
testImplementation(commonLibs.hypertrace.grpcutils.client)
}
Expand Down
20 changes: 13 additions & 7 deletions config-service-impl/gradle.lockfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,22 @@ org.hypertrace.core.kafkastreams.framework:kafka-bom:0.5.4=compileClasspath,runt
org.hypertrace.core.serviceframework:docstore-metrics:0.1.86=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
org.hypertrace.core.serviceframework:platform-metrics:0.1.86=runtimeClasspath,testRuntimeClasspath
org.hypertrace.core.serviceframework:service-framework-spi:0.1.86=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
org.junit.jupiter:junit-jupiter-api:5.10.0=testCompileClasspath,testRuntimeClasspath
org.junit.jupiter:junit-jupiter-engine:5.10.0=testRuntimeClasspath
org.junit.jupiter:junit-jupiter-params:5.10.0=testCompileClasspath,testRuntimeClasspath
org.junit.jupiter:junit-jupiter:5.10.0=testCompileClasspath,testRuntimeClasspath
org.junit.platform:junit-platform-commons:1.10.0=testCompileClasspath,testRuntimeClasspath
org.junit.platform:junit-platform-engine:1.10.0=testRuntimeClasspath
org.junit:junit-bom:5.10.0=testCompileClasspath,testRuntimeClasspath
org.junit.jupiter:junit-jupiter-api:5.10.0=testCompileClasspath
org.junit.jupiter:junit-jupiter-api:5.10.1=testRuntimeClasspath
org.junit.jupiter:junit-jupiter-engine:5.10.1=testRuntimeClasspath
org.junit.jupiter:junit-jupiter-params:5.10.0=testCompileClasspath
org.junit.jupiter:junit-jupiter-params:5.10.1=testRuntimeClasspath
org.junit.jupiter:junit-jupiter:5.10.0=testCompileClasspath
org.junit.jupiter:junit-jupiter:5.10.1=testRuntimeClasspath
org.junit.platform:junit-platform-commons:1.10.0=testCompileClasspath
org.junit.platform:junit-platform-commons:1.10.1=testRuntimeClasspath
org.junit.platform:junit-platform-engine:1.10.1=testRuntimeClasspath
org.junit:junit-bom:5.10.0=testCompileClasspath
org.junit:junit-bom:5.10.1=testRuntimeClasspath
org.latencyutils:LatencyUtils:2.0.3=runtimeClasspath,testRuntimeClasspath
org.lz4:lz4-java:1.8.0=runtimeClasspath,testRuntimeClasspath
org.mockito:mockito-core:5.8.0=testCompileClasspath,testRuntimeClasspath
org.mockito:mockito-junit-jupiter:5.8.0=testCompileClasspath,testRuntimeClasspath
org.mongodb:bson-record-codec:5.2.0=runtimeClasspath,testRuntimeClasspath
org.mongodb:bson:5.2.0=runtimeClasspath,testRuntimeClasspath
org.mongodb:mongodb-driver-core:5.2.0=runtimeClasspath,testRuntimeClasspath
Expand Down
180 changes: 53 additions & 127 deletions ...rvice-impl/src/test/java/org/hypertrace/config/service/store/DocumentConfigStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.hypertrace.config.service.store;

import static java.util.Collections.emptyList;
import static org.hypertrace.config.service.TestUtils.CONTEXT1;
import static org.hypertrace.config.service.TestUtils.RESOURCE_NAME;
import static org.hypertrace.config.service.TestUtils.RESOURCE_NAMESPACE;
Expand All @@ -12,6 +13,7 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -53,14 +55,19 @@
import org.hypertrace.core.documentstore.expression.impl.LogicalExpression;
import org.hypertrace.core.documentstore.expression.impl.RelationalExpression;
import org.hypertrace.core.documentstore.expression.type.FilterTypeExpression;
import org.hypertrace.core.documentstore.metric.DocStoreMetricProvider;
import org.hypertrace.core.documentstore.model.options.QueryOptions;
import org.hypertrace.core.documentstore.query.Query;
import org.hypertrace.core.documentstore.query.SortingSpec;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Answers;
import org.mockito.ArgumentCaptor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class DocumentConfigStoreTest {

private static final long CONFIG_VERSION = 5;
Expand All @@ -72,17 +79,19 @@ class DocumentConfigStoreTest {
private static Value config1 = getConfig1();
private static Value config2 = getConfig2();
private static ConfigResourceContext configResourceContext = getConfigResourceContext();
private static Collection collection;
private DocumentConfigStore configStore;
private Clock mockClock;
private FilterBuilder filterBuilder;

@BeforeEach()
private Collection collection;
@Mock private Clock mockClock;
Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Apr 23, 2025

Choose a reason for hiding this comment

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

So I added the mockito extension just as a best practice, it's not directly related to the change. But it does lend readability as we need less setup and it will fail if we start mocking things that the test path doesn't touch (you can see some unused mocks I removed in WriteConfigForUpdateWithUpsertCondition)

@Mock private FilterBuilder filterBuilder;

@Mock(answer = Answers.RETURNS_DEEP_STUBS)
private Datastore mockDatastore;

@InjectMocks private DocumentConfigStore configStore;

@BeforeEach
void beforeEach() {
collection = mock(Collection.class);
this.mockClock = mock(Clock.class);
this.filterBuilder = mock(FilterBuilder.class);
this.configStore = new DocumentConfigStore(mockClock, new MockDatastore());
this.collection = mockDatastore.getCollection(CONFIGURATIONS_COLLECTION);
}

@Test
Expand Down Expand Up @@ -192,10 +201,7 @@ void WriteConfigForUpdateWithUpsertCondition() throws IOException {
.setValue(Values.of("v2")))))
.build();

org.hypertrace.core.documentstore.Filter docFilter =
new org.hypertrace.core.documentstore.Filter();
when(request.getUpsertCondition()).thenReturn(upsertCondition);
when(filterBuilder.buildDocStoreFilter(upsertCondition)).thenReturn(docFilter);
UpsertedConfig upsertedConfig =
configStore.writeConfig(configResourceContext, USER_ID, request, USER_EMAIL);
assertEquals(config2, upsertedConfig.getConfig());
Expand All @@ -222,7 +228,6 @@ void WriteConfigForUpdateWithUpsertCondition() throws IOException {
document);

// failed upsert condition
when(updateResult.getUpdatedCount()).thenReturn(0L);
assertThrows(
StatusRuntimeException.class,
() -> configStore.writeConfig(configResourceContext, USER_ID, request, USER_EMAIL));
Expand Down Expand Up @@ -283,7 +288,7 @@ void getAllConfigs() throws IOException {
new ConfigResource(RESOURCE_NAME, RESOURCE_NAMESPACE, TENANT_ID),
Filter.getDefaultInstance(), // for empty filter
Pagination.getDefaultInstance(),
Collections.emptyList());
emptyList());

assertEquals(2, contextSpecificConfigList.size());
assertEquals(
Expand Down Expand Up @@ -370,30 +375,6 @@ void writeAllConfigs() throws IOException {

@Test
void buildQuery_withDefaultPagination() throws Exception {
// Arrange
ConfigResource configResource =
new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1");
Filter filter = Filter.getDefaultInstance();
Pagination pagination = Pagination.getDefaultInstance(); // Default pagination

// Create a mock for CloseableIterator<Document>
CloseableIterator<Document> mockIterator = mock(CloseableIterator.class);
when(mockIterator.hasNext()).thenReturn(false); // No documents to process

// Mock the collection.query to return the mock iterator
when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator);

// Act
configStore.getAllConfigs(configResource, filter, pagination, Collections.emptyList());

// Capture the Query object passed to collection.query
ArgumentCaptor<Query> queryCaptor = ArgumentCaptor.forClass(Query.class);
verify(collection).query(queryCaptor.capture(), any(QueryOptions.class));

// Assert the properties of the captured Query
Query capturedQuery = queryCaptor.getValue();
assertNotNull(capturedQuery);

List<SortingSpec> expectedSorts =
Arrays.asList(
SortingSpec.of(
Expand All @@ -402,47 +383,39 @@ void buildQuery_withDefaultPagination() throws Exception {
SortingSpec.of(
IdentifierExpression.of("creationTimestamp"),
org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC));
assertEquals(expectedSorts, capturedQuery.getSorts());

configStore.getAllConfigs(
new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"),
Filter.getDefaultInstance(),
Pagination.getDefaultInstance(),
emptyList());

verify(collection)
Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Apr 23, 2025

Choose a reason for hiding this comment

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

This is what I was suggesting in the other PR - arg captors aren't needed, just call the public API and verify that the query looks like you want it to.

.query(
argThat(query -> query.getSorts().equals(expectedSorts)),
any(QueryOptions.class));
}

@Test
void buildQuery_withCustomPagination() throws IOException {
ConfigResource configResource =
new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1");
Filter filter = Filter.getDefaultInstance();
Pagination pagination = Pagination.newBuilder().setOffset(10).setLimit(20).build();
List<org.hypertrace.config.service.v1.SortBy> sortByList = Collections.emptyList();

// Create a mock for CloseableIterator<Document>
CloseableIterator<Document> mockIterator = mock(CloseableIterator.class);
when(mockIterator.hasNext()).thenReturn(false); // No documents to process

// Mock the collection.query to return the mock iterator
when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator);

// Act
configStore.getAllConfigs(configResource, filter, pagination, sortByList);

// Capture the Query object passed to collection.query
ArgumentCaptor<Query> queryCaptor = ArgumentCaptor.forClass(Query.class);
verify(collection).query(queryCaptor.capture(), any(QueryOptions.class));

// Assert the properties of the captured Query
Query capturedQuery = queryCaptor.getValue();
assertNotNull(capturedQuery);

org.hypertrace.core.documentstore.query.Pagination expectedPagination =
org.hypertrace.core.documentstore.query.Pagination.builder().offset(10).limit(20).build();
assertEquals(expectedPagination, capturedQuery.getPagination().get());

configStore.getAllConfigs(
new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"),
Filter.getDefaultInstance(),
pagination,
emptyList());

verify(collection)
.query(
argThat(query -> query.getPagination().orElseThrow().equals(expectedPagination)),
any(QueryOptions.class));
}

@Test
void buildQuery_withCustomSorting() throws IOException {
ConfigResource configResource =
new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1");
Filter filter = Filter.getDefaultInstance();
Pagination pagination = Pagination.getDefaultInstance();

org.hypertrace.config.service.v1.Selection selection =
org.hypertrace.config.service.v1.Selection.newBuilder()
.setConfigJsonPath("test.path")
Expand All @@ -452,24 +425,7 @@ void buildQuery_withCustomSorting() throws IOException {
.setSelection(selection)
.setSortOrder(SortOrder.SORT_ORDER_DESC)
.build();
List<org.hypertrace.config.service.v1.SortBy> sortByList = Collections.singletonList(sortBy);
// Create a mock for CloseableIterator<Document>
CloseableIterator<Document> mockIterator = mock(CloseableIterator.class);
when(mockIterator.hasNext()).thenReturn(false); // No documents to process

// Mock the collection.query to return the mock iterator
when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator);

// Act
configStore.getAllConfigs(configResource, filter, pagination, sortByList);

// Capture the Query object passed to collection.query
ArgumentCaptor<Query> queryCaptor = ArgumentCaptor.forClass(Query.class);
verify(collection).query(queryCaptor.capture(), any(QueryOptions.class));

// Assert the properties of the captured Query
Query capturedQuery = queryCaptor.getValue();
assertNotNull(capturedQuery);
List<SortingSpec> expectedSorts =
Arrays.asList(
SortingSpec.of(
Expand All @@ -479,7 +435,14 @@ void buildQuery_withCustomSorting() throws IOException {
IdentifierExpression.of("config.test.path"),
org.hypertrace.core.documentstore.expression.operators.SortOrder.DESC));

assertEquals(expectedSorts, capturedQuery.getSorts());
configStore.getAllConfigs(
new ConfigResource(RESOURCE_NAMESPACE, RESOURCE_NAME, "tenant1"),
Filter.getDefaultInstance(),
Pagination.getDefaultInstance(),
Collections.singletonList(sortBy));

verify(collection)
.query(argThat(query -> query.getSorts().equals(expectedSorts)), any(QueryOptions.class));
}

@Test
Expand All @@ -497,7 +460,7 @@ void buildQuery_withFilter() throws IOException {
Filter filter = Filter.newBuilder().setRelationalFilter(relationalFilter).build();

Pagination pagination = Pagination.getDefaultInstance();
List<org.hypertrace.config.service.v1.SortBy> sortByList = Collections.emptyList();
List<org.hypertrace.config.service.v1.SortBy> sortByList = emptyList();

// Create a mock for CloseableIterator<Document>
CloseableIterator<Document> mockIterator = mock(CloseableIterator.class);
Expand All @@ -507,7 +470,7 @@ void buildQuery_withFilter() throws IOException {
when(collection.query(any(Query.class), any(QueryOptions.class))).thenReturn(mockIterator);

// Act
configStore.getAllConfigs(configResource, filter, pagination, Collections.emptyList());
configStore.getAllConfigs(configResource, filter, pagination, emptyList());

// Capture the Query object passed to collection.query
ArgumentCaptor<Query> queryCaptor = ArgumentCaptor.forClass(Query.class);
Expand Down Expand Up @@ -540,7 +503,7 @@ private FilterTypeExpression createExpectedFilter() {
lhs, org.hypertrace.core.documentstore.expression.operators.RelationalOperator.EQ, rhs);
}

private static Document getConfigDocument(
private Document getConfigDocument(
String context, long version, Value config, long creationTimestamp, long updateTimestamp) {
return new ConfigDocument(
RESOURCE_NAME,
Expand Down Expand Up @@ -577,41 +540,4 @@ public Document next() {
return documentIterator.next();
}
}

public static class MockDatastore implements Datastore {
Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Apr 23, 2025

Choose a reason for hiding this comment

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

This was overkill, a regular mockito mock can do this more simply.


@Override
public Set<String> listCollections() {
return Collections.singleton(CONFIGURATIONS_COLLECTION);
}

@Override
public Collection getCollection(String s) {
return collection;
}

// default implementation for other methods as they are unused
@Override
public boolean createCollection(String s, Map<String, String> map) {
return false;
}

@Override
public boolean deleteCollection(String s) {
return false;
}

@Override
public boolean healthCheck() {
return false;
}

@Override
public DocStoreMetricProvider getDocStoreMetricProvider() {
return null;
}

@Override
public void close() {}
}
}
Loading