-
Notifications
You must be signed in to change notification settings - Fork 829
Speed up BATS tests #4478
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.
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
base: main
Are you sure you want to change the base?
Speed up BATS tests #4478
Changes from all commits
6016131
62ac939
e5722f4
064e347
3bf997e
c5f15a9
127ba60
8b13e5b
6de8b12
9aadafd
e94e174
29f3cca
abea3e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| /* | ||
| * 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.servlet; | ||
|
|
||
| import org.apache.solr.client.solrj.request.CollectionAdminRequest; | ||
| import org.apache.solr.client.solrj.request.UpdateRequest; | ||
| import org.apache.solr.cloud.SolrCloudTestCase; | ||
| import org.apache.solr.common.SolrInputDocument; | ||
| import org.eclipse.jetty.client.ContentResponse; | ||
| import org.eclipse.jetty.client.HttpClient; | ||
| import org.eclipse.jetty.http.HttpHeader; | ||
| import org.junit.AfterClass; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * Verifies that Solr's Jetty GzipHandler correctly compresses HTTP responses when the client sends | ||
| * {@code Accept-Encoding: gzip} and omits compression when that header is absent. | ||
| */ | ||
| public class GzipCompressionTest extends SolrCloudTestCase { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I miss the succinteness of the .bats test and how easy it is to read (to my eyes at least!), but totally understand the overhead.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we keep adding BATS tests in the pace we have been doing lately, we'll surpass 60m run time very soon. So we should probably reserve BATS for testing our bin/solr shell script, plus mechanisms that won't kick in in unit tests. The java-agent is an example of such. And "realistic" jetty setup is probably another. But I'd hope that as we succeed in slimming down bin/solr even more and move things into java-land, we should be able to migrate more BATS tests to JUnit since the logic is no longer done in bash. We have already delegated lots of SolrCLI arg parsing to java-land. There is still some glue in script that construct the java cmdline, which do need to be tested, but the cmdline arg/opts parsing for each tool should not need full bats coverage if we know we have separate junit coverage of such parsing. |
||
|
|
||
| private static final String COLLECTION = "gzip-test"; | ||
|
|
||
| @BeforeClass | ||
| public static void setupCluster() throws Exception { | ||
| // Lower the minGzipSize threshold so any non-empty response body is eligible for compression. | ||
| // jetty-gzip.xml reads this property at Jetty start time via <Property | ||
| // name="jetty.gzip.minGzipSize">. | ||
| System.setProperty("jetty.gzip.minGzipSize", "1"); | ||
|
|
||
| configureCluster(1).configure(); | ||
|
|
||
| CollectionAdminRequest.createCollection(COLLECTION, null, 1, 1) | ||
| .process(cluster.getSolrClient()); | ||
|
|
||
| // Index a few documents so the select response has a non-trivial body | ||
| UpdateRequest req = new UpdateRequest(); | ||
| for (int i = 0; i < 10; i++) { | ||
| SolrInputDocument doc = new SolrInputDocument(); | ||
| doc.addField("id", "doc-" + i); | ||
| doc.addField("name_s", "Document number " + i + " for gzip compression test"); | ||
| req.add(doc); | ||
| } | ||
| req.commit(cluster.getSolrClient(), COLLECTION); | ||
| } | ||
|
|
||
| @AfterClass | ||
| public static void clearGzipSysProp() { | ||
| System.clearProperty("jetty.gzip.minGzipSize"); | ||
| } | ||
|
|
||
| private HttpClient getHttpClient() { | ||
| return cluster.getJettySolrRunner(0).getSolrClient().getHttpClient(); | ||
| } | ||
|
|
||
| private String getSelectUrl() { | ||
| return cluster.getJettySolrRunner(0).getBaseUrl() + "/" + COLLECTION + "/select?q=*:*&rows=10"; | ||
| } | ||
|
|
||
| @Test | ||
| public void testNoCompressionWithoutAcceptEncodingHeader() throws Exception { | ||
| ContentResponse response = getHttpClient().GET(getSelectUrl()); | ||
| assertEquals("Expected HTTP 200", 200, response.getStatus()); | ||
| assertNull( | ||
| "Content-Encoding should not be set when Accept-Encoding header is absent", | ||
| response.getHeaders().get(HttpHeader.CONTENT_ENCODING)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGzipCompressionWithAcceptEncodingHeader() throws Exception { | ||
| ContentResponse response = | ||
| getHttpClient() | ||
| .newRequest(getSelectUrl()) | ||
| .headers(h -> h.add(HttpHeader.ACCEPT_ENCODING, "gzip")) | ||
| .send(); | ||
| assertEquals("Expected HTTP 200", 200, response.getStatus()); | ||
| assertEquals( | ||
| "Expected gzip Content-Encoding when Accept-Encoding: gzip was requested", | ||
| "gzip", | ||
| response.getHeaders().get(HttpHeader.CONTENT_ENCODING)); | ||
| } | ||
| } | ||
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would use need SolrCloud for only a jetty level thing?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solrcloud is the default now, why not - it's a 1-node "cluster". Is there some overhead with SolrCloudTestCase that we don't have with a simpler JettySolrRunner?
I also tried to refactor to
JettySolrRunnerbut it has no GZip handler as the cloud setup does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is the principle of using the leanest test infrastructure to accomplish the testing goal. Default doesn't matter.
Where is the gzip support in the SolrCloudTestCase/MiniSolrCloudCluster you speak of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have to dive into the test setup to answer. I thought we used same JettySolrRunner as in standalone, and that Jetty is configured in java instead of xml, but still not 100% identical. I have not verified that the new Gzip test actually tests what it claims to. Will have to do a manual dive to verify. I had my hopes up when Claude selected it as a low hanging fruit.