gerlowskija commented on code in PR #975: URL: https://github.com/apache/solr/pull/975#discussion_r965213364
########## solr/core/src/test/org/apache/solr/handler/configsets/ListConfigSetsAPITest.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.handler.configsets; + +import static org.apache.solr.SolrTestCaseJ4.assumeWorkingMockito; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.List; +import javax.inject.Singleton; +import javax.ws.rs.core.Application; +import javax.ws.rs.core.Response; +import org.apache.solr.client.solrj.response.ConfigSetAdminResponse; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.core.ConfigSetService; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.api.V2ApiUtils; +import org.apache.solr.jersey.CoreContainerFactory; +import org.apache.solr.jersey.SolrJacksonMapper; +import org.glassfish.hk2.utilities.binding.AbstractBinder; +import org.glassfish.jersey.server.ResourceConfig; +import org.glassfish.jersey.test.JerseyTest; +import org.junit.BeforeClass; +import org.junit.Test; + +public class ListConfigSetsAPITest extends JerseyTest { + + private CoreContainer mockCoreContainer; + + @BeforeClass + public static void ensureWorkingMockito() { + assumeWorkingMockito(); + } + + @Override + protected Application configure() { + resetMocks(); + final ResourceConfig config = new ResourceConfig(); + config.register(ListConfigSetsAPI.class); + config.register(SolrJacksonMapper.class); + config.register( + new AbstractBinder() { + @Override + protected void configure() { + bindFactory(new CoreContainerFactory(mockCoreContainer)) + .to(CoreContainer.class) + .in(Singleton.class); + } + }); + + return config; + } + + private void resetMocks() { + mockCoreContainer = mock(CoreContainer.class); + } + + @Test + public void testSuccessfulListConfigsetsRaw() throws Exception { + final String expectedJson = + "{\"responseHeader\":{\"status\":0,\"QTime\":0},\"configSets\":[\"cs1\",\"cs2\"]}"; + final ConfigSetService configSetService = mock(ConfigSetService.class); + when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService); + when(configSetService.listConfigs()).thenReturn(List.of("cs1", "cs2")); + + final Response response = target("/cluster/configs").request().get(); + final String jsonBody = response.readEntity(String.class); + + assertEquals(200, response.getStatus()); + assertEquals("application/json", response.getHeaders().getFirst("Content-type")); + assertEquals(1, 1); + assertEquals( + expectedJson, + "{\"responseHeader\":{\"status\":0,\"QTime\":0},\"configSets\":[\"cs1\",\"cs2\"]}"); + } + + @Test + public void testSuccessfulListConfigsetsTyped() throws Exception { + final ConfigSetService configSetService = mock(ConfigSetService.class); + when(mockCoreContainer.getConfigSetService()).thenReturn(configSetService); + when(configSetService.listConfigs()).thenReturn(List.of("cs1", "cs2")); + + final ListConfigsetsResponse response = + target("/cluster/configs").request().get(ListConfigsetsResponse.class); + + assertNotNull(response.configSets); + assertNull(response.error); + assertEquals(2, response.configSets.size()); + assertTrue(response.configSets.contains("cs1")); + assertTrue(response.configSets.contains("cs2")); + } + + /** + * Test the v2 to v1 response mapping for /cluster/configs + * + * <p>{@link org.apache.solr.handler.admin.ConfigSetsHandler} uses {@link ListConfigSetsAPI} (and + * its response class {@link ListConfigsetsResponse}) internally to serve the v1 version of this + * functionality. So it's important to make sure that the v2 response stays compatible with SolrJ + * - both because that's important in its own right and because that ensures we haven't + * accidentally changed the v1 response format. + */ + @Test + public void testListConfigsetsV1Compatibility() throws Exception { Review Comment: Hah, the old integration vs unit test debate. Each has a place at the table IMO. Integration tests do give you better fidelity for high level functionality. I wouldn't rely on unit tests alone. And unit tests do often need to change with the code, which is a pain. But IMO unit tests have a lot of advantages: they're faster, more targeted, run less risk of unrelated environmental issues (such as cause many of Solr's flaky build issues), allow testing of otherwise hard-to-trigger codepaths, etc. To answer your specific question about this test, I thought a unit test would be a good addition for two reasons: First, Solr has lots of integration tests around configset-listing, and I trust those to flag issues with json/xml/javabin serialization if this were to break. But none of them are very focused on serialization. They each leave a pretty big gap between seeing the failure and understanding the root cause and fixing it. This test addresses that by having something very narrowly focused on just the response conversions involved, hopefully making a failure much more actionable. Second, I wanted to create an example test case based on a Jersey test-application. The relative simplicity of the list-configset API doesn't make the point very well, but more complex APIs will benefit a lot from Jersey-framework-aided unit testing. And I wanted to include an example of that in this PR. If neither of those reasons are convincing for you though, I'm happy to rip it out for now and rely on the existing list-configset tests. (Deferring the example jersey-framework test, until we convert a v2 API complex enough to require it) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
