Re: [PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
Copilot commented on code in PR #4227:
URL: https://github.com/apache/solr/pull/4227#discussion_r3279730078
##
test-external-client/src/test/java/DependencySmokeTest.java:
##
@@ -0,0 +1,42 @@
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.util.EmbeddedSolrServerTestRule;
+import org.apache.solr.util.SolrClientTestRule;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+public class DependencySmokeTest extends SolrTestCase {
+
+ @ClassRule
+ public static SolrClientTestRule solrRule = new EmbeddedSolrServerTestRule();
+
+ @Test
+ public void testSolrjAndTestFrameworkAreUsable() throws SolrServerException,
IOException {
+solrRule.startSolr();
+// TODO solr-test-framework.jar or maybe solr-core.jar ought to include
the _default configSet
+// and maybe a cloud-minimal configset. Once it does, then let's see if
we can enhance
+// withConfigSet to handle JAR Paths, and add a static getFile perhaps to
make it easy.
+Path configSet =
Path.of("../solr/server/solr/configsets/_default/").toAbsolutePath();
+solrRule.newCollection().withConfigSet(configSet).create();
+solrRule.getSolrClient().ping();
Review Comment:
This test hard-codes a configset path outside the test-external-client
project (../solr/server/solr/configsets/_default). That makes the project
non-standalone and will fail in the Docker-based Maven run (smokeTestRelease
mounts only test-external-client at /project, so ../solr/... doesn’t exist).
Consider vendoring a minimal configset under test-external-client (e.g.,
src/test/resources/configsets/...) and resolving it relative to the
project/test resources instead of the repo checkout.
##
test-external-client/README.md:
##
@@ -0,0 +1,44 @@
+# Solr dependency smoke test (Maven + Gradle)
+
+This mini project validates that a standalone build can resolve Solr artifacts
from a Maven-style local directory tree and run a basic test using them.
+
+## Inputs
+
+- `solr.version` (required): version of `solr-solrj` and `solr-test-framework`
+- `local.solr.repo` (optional): filesystem path to a Maven-layout repository
(default: `~/.m2/repository`)
+
Review Comment:
The README states the default for local.solr.repo is ~/.m2/repository, but
the Gradle build defaults local.solr.repo to $user.dir/build/maven-local.
Please reconcile/clarify the defaults (and note Maven vs Gradle differences if
intentional) to avoid confusing users running the smoke test.
##
dev-tools/scripts/smokeTestRelease.py:
##
@@ -788,6 +788,66 @@ def testSolrExample(binaryDistPath, javaPath, isSlim):
os.chdir(old_cwd)
+def findMaven():
+ """Find the mvn executable in PATH. Returns the command path, or None if not
found."""
+ import shutil as shutil_util
+ return shutil_util.which('mvn')
+
+
+def _dockerAvailable():
+ """Check whether Docker is installed and the daemon is running."""
+ import shutil as shutil_util
+ if shutil_util.which('docker') is None:
+return False
+ return os.system('docker info > /dev/null 2>&1') == 0
+
+
+def testMavenBuild(mavenDir, tmpDir, version):
+ """
+ Runs the test-external-client project with both Maven and Gradle to verify
that the
+ published POMs for solr-solrj and solr-test-framework declare correct
transitive
+ dependencies.
+
+ mavenDir: root of the local Maven repository (contains org/apache/solr/...)
+ tmpDir: temp directory for log files
+ version: Solr version string (e.g. "10.0.0")
+ """
+ print('test external client project (verify POMs are consumable)...')
+
+ scriptDir = os.path.dirname(os.path.abspath(__file__))
+ projectDir = os.path.normpath(os.path.join(scriptDir, '..', '..',
'test-external-client'))
+ if not os.path.isdir(projectDir):
+raise RuntimeError('test-external-client directory not found at: %s' %
projectDir)
+
+ # Run Maven build
+ mvnCmd = findMaven()
+ if mvnCmd is not None:
+print(' using local Maven: %s' % mvnCmd)
+run('%s -B -f "%s/pom.xml" -Dsolr.version="%s" -Dlocal.sol
Re: [PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
janhoy commented on code in PR #4227:
URL: https://github.com/apache/solr/pull/4227#discussion_r3279579699
##
dev-tools/scripts/smokeTestRelease.py:
##
@@ -633,7 +633,7 @@ def verifyUnpacked(java, artifact, unpackPath, gitRevision,
version, testArgs):
# in_root_folder.remove(fileName)
if isSrc:
-expected_src_root_folders = ['build-tools', 'changelog', 'dev-docs',
'dev-tools', 'gradle', 'solr']
+expected_src_root_folders = ['build-tools', 'changelog', 'dev-docs',
'dev-tools', 'gradle', 'solr', 'test-external-client']
Review Comment:
PLease bury this test-code deeper in the file hierarchy..
##
test-external-client/build.gradle.kts:
##
@@ -0,0 +1,66 @@
+/*
+ * 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.
+ */
+
+plugins {
+java
+}
+
+group = "org.apache.solr"
+version = "1.0-SNAPSHOT"
+description = "A smoke test for solr-test-framework & SolrJ dependencies and
usage."
+
+val solrVersion =
providers.gradleProperty("solr.version").orElse("11.0.0-SNAPSHOT")
Review Comment:
Fail if not solr.version is set? A hassle to maintain hard coded version
strings which must be bumped on every major release... better to always require
version being set.
##
dev-tools/scripts/smokeTestRelease.py:
##
@@ -809,6 +869,8 @@ def checkMaven(baseURL, tmpDir, gitRevision, version,
isSigned, keysFile):
checkAllJARs('%s/maven/org/apache/solr' % tmpDir, gitRevision, version)
+ testMavenBuild('%s/maven' % tmpDir, tmpDir, version)
Review Comment:
Should there be an opt-in or opt-out CLI option for this test? I think I
prefer opt-out, for which the smoke tester should fail-fast if mvn or docker is
not available, unless user has opted out of this test. I don't like silently
skipping due to env.
##
dev-tools/scripts/smokeTestRelease.py:
##
@@ -788,6 +788,66 @@ def testSolrExample(binaryDistPath, javaPath, isSlim):
os.chdir(old_cwd)
+def findMaven():
+ """Find the mvn executable in PATH. Returns the command path, or None if not
found."""
+ import shutil as shutil_util
+ return shutil_util.which('mvn')
+
+
+def _dockerAvailable():
+ """Check whether Docker is installed and the daemon is running."""
+ import shutil as shutil_util
+ if shutil_util.which('docker') is None:
+return False
+ return os.system('docker info > /dev/null 2>&1') == 0
+
+
+def testMavenBuild(mavenDir, tmpDir, version):
+ """
+ Runs the test-external-client project with both Maven and Gradle to verify
that the
+ published POMs for solr-solrj and solr-test-framework declare correct
transitive
+ dependencies.
+
+ mavenDir: root of the local Maven repository (contains org/apache/solr/...)
+ tmpDir: temp directory for log files
+ version: Solr version string (e.g. "10.0.0")
+ """
+ print('test external client project (verify POMs are consumable)...')
+
+ scriptDir = os.path.dirname(os.path.abspath(__file__))
+ projectDir = os.path.normpath(os.path.join(scriptDir, '..', '..',
'test-external-client'))
Review Comment:
Is this correct? You typically run the smoke tester from a local git
checkout, the smoke tester downloads an RC, places it in some
`/tmp/solr-smoketest-xyz/...` folder in which there are solr tgz, -src.tgz,
maven/ folder etc. This line will use the local `test-external-client` rather
than the version contained in the unpacked src-tgz?
##
test-external-client/pom.xml:
##
@@ -0,0 +1,85 @@
+
+
+http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
https://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ 4.0.0
+
+ org.apache.solr
+ test-external-client
+ 1.0-SNAPSHOT
+ Solr Dependency Smoke Test
+
+
+UTF-8
+21
+${user.home}/.m2/repository
+11.0.0-SNAPSHOT
Review Comment:
Same comment as for gradle, require that prop to be passed in always, or
else fail.
--
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: [email protected]
For queries about this
Re: [PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
dsmiley commented on PR #4227: URL: https://github.com/apache/solr/pull/4227#issuecomment-4110980742 BTW I generalized the scope. The PR title suggests this is only about published POMs. That is one thing this little project _indirectly_ checks, the other thing that is checked is that solr-test-framework can actually be used externally. Hence I called this dir "test-external-client". -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
dsmiley commented on PR #4227: URL: https://github.com/apache/solr/pull/4227#issuecomment-411025 I could see that Docker could be used to execute the build of this little project in order to provide total detachment/isolation from the source tree, thus making it needless for me to be a little clever in ExternalPaths regarding the leading "solr/". -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
dsmiley commented on code in PR #4227:
URL: https://github.com/apache/solr/pull/4227#discussion_r2974628066
##
solr/test-framework/src/java/org/apache/solr/util/ExternalPaths.java:
##
@@ -83,10 +83,13 @@ static Path determineSourceHome() {
}
Path base = file.toAbsolutePath();
- while (!Files.exists(base.resolve("solr/test-framework/build.gradle"))
&& null != base) {
Review Comment:
This is a subtle difference that I bet nobody will notice. Not the null
check but the path. Notice I removed the leading "solr/" component, and then
some lines below here, I removed adding the "solr/" back on when we return, as
it becomes pointless. On one hand, this subtle difference is a
micro-optimization -- why iterate to the root of the project when we can
iterate and stop at the "solr/" child and then reach success one iteration
sooner. My _real_ motivation is that this is absolutely **critical** for
`test-external-client`, which uses `ExternalPaths`. We want that project to
experience what any project outside Solr would see -- null for all paths here.
So @epugh that answers why it's critical that `test-external-client` be at the
root, or at least *not* inside `solr/`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
janhoy commented on PR #4227: URL: https://github.com/apache/solr/pull/4227#issuecomment-4110011687 > It would be nice if running this was a constant thing, not a "and now it's release time" type thing. Wrt maven files, it is most crucial to avoid shipping something broken, so I'm good with release-time. But I also agree that if it was wrappable in a gradle target, then any change to gradle files could trigger such a test in github actions. Since it involves actually running `mvn` I suspect it becomes awkward to do this inside JUnit framework, but perhaps as a TestContainer that depends on first building maven, lots of things could be done in that container. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
epugh commented on PR #4227: URL: https://github.com/apache/solr/pull/4227#issuecomment-4109938907 I really like the upping of our game in validating things. I wish we had a more general way of handling lots of kinds of "integration" or "system" tests like this one. We have some emerging tests in our bats infrastructure, and then some of these smokTest things like this. Could this test have been based on a Docker based setup so that if I don't have a specific version of maven and gradle installed, that it would still work? It would be nice if running this was a constant thing, not a "and now it's release time" type thing. I also am not sure about more and more directories at the root.. Could `test-external-client` be part of some sort of `solr/system-tests`? Or `solr/smoke-tests`? With other heavy/complex tests? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
[PR] SOLR-18069: Add Maven build smoke test to verify published POMs [solr]
dsmiley opened a new pull request, #4227: URL: https://github.com/apache/solr/pull/4227 https://issues.apache.org/jira/browse/SOLR-18069 Original PR in my fork done by CoPilot Agent: https://github.com/dsmiley/solr/pull/21 Didn't test/run this and I don't know what to think of the code. This is just a draft PR after all ;-) I love that the maven aspects are factored out and individually runnable. TBH I'm the last one to be writing python here. Ideally someone immediately takes over this. Or tells me to close this pile of crap ;-) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
