dsmiley commented on code in PR #3300:
URL: https://github.com/apache/solr/pull/3300#discussion_r2022994072
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java:
##########
@@ -376,26 +377,26 @@ public void testUpConfigLinkConfigClearZk() throws
Exception {
assertEquals(confsetname, collectionProps.getStr("configName"));
// test down config
- Path configSetDir =
- tmpDir.resolve(
- "solrtest-confdropspot-" + this.getClass().getName() + "-" +
System.nanoTime());
- assertFalse(Files.exists(configSetDir));
+ Path destDir =
Review Comment:
A clearer name, as this test deals with many configSetDirs, as input and
output.
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java:
##########
@@ -406,45 +407,35 @@ public void testUpConfigLinkConfigClearZk() throws
Exception {
assertEquals("Comparing downloaded files to what is in ZK",
files.size(), zkFiles.size());
}
- Path sourceConfDir = ExternalPaths.TECHPRODUCTS_CONFIGSET;
- // filter out all directories starting with . (e.g. .svn)
- try (Stream<Path> stream = Files.walk(sourceConfDir)) {
- List<Path> files =
- stream
- .filter(Files::isRegularFile)
- .filter(path -> path.getFileName().startsWith("."))
Review Comment:
There were a comedy of errors in this test; the "worst" one was here, having
multiple flaws. Firstly, `filter` chooses those matching the condition;
someone forgot to invert this! Thus the test didn't loop over anything.
Secondly, the interpretation of this method is for "." to be a path, since we
are calling Path.startsWith *not* String.startsWith. IMO the JDK developers
chose a terribly confusing name that is forbidden-api worthy using "startsWith"
taking a String. So any way, this ends up sort of kind of being, filter to
paths that are under the current directory as that's what "." resolves to
(CWD)... but only if normalize().toAbsolute() were to be called; which it
wasn't and we don't want any way since this wasn't the actual intention.
In the end I dropped the intended filter since I don't see that we would
encounter a dot-file.
CC @epugh (original author) @mlbiscoc (latest author)
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java:
##########
@@ -406,45 +407,35 @@ public void testUpConfigLinkConfigClearZk() throws
Exception {
assertEquals("Comparing downloaded files to what is in ZK",
files.size(), zkFiles.size());
}
- Path sourceConfDir = ExternalPaths.TECHPRODUCTS_CONFIGSET;
- // filter out all directories starting with . (e.g. .svn)
- try (Stream<Path> stream = Files.walk(sourceConfDir)) {
- List<Path> files =
- stream
- .filter(Files::isRegularFile)
- .filter(path -> path.getFileName().startsWith("."))
- .toList();
- files.forEach(
- (sourceFile) -> {
- int indexOfRelativePath =
- sourceFile
- .toString()
- .lastIndexOf(
- "sample_techproducts_configs"
- + sourceFile.getFileSystem().getSeparator()
- + "conf");
- String relativePathofFile =
sourceFile.toString().substring(indexOfRelativePath + 33);
Review Comment:
I should have caught this in a code review, as it screams of pre-Path
thinking. It's highly suspicious to be doing string.indexOf when we transition
to the Path API. My replacement uses path.relativize()
--
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
