dsmiley commented on code in PR #3263:
URL: https://github.com/apache/solr/pull/3263#discussion_r2001724020
##########
solr/core/src/test/org/apache/solr/cli/SolrCLIZkToolsTest.java:
##########
@@ -131,12 +134,11 @@ public void testDownconfig() throws Exception {
ConfigSetDownloadTool downTool = new ConfigSetDownloadTool();
int res = downTool.runTool(SolrCLI.processCommandLineArgs(downTool, args));
assertEquals("Download should have succeeded.", 0, res);
- verifyZkLocalPathsMatch(
- Paths.get(tmp.toAbsolutePath().toString(), "conf"),
"/configs/downconfig1");
+ verifyZkLocalPathsMatch(tmp.toAbsolutePath().resolve("conf"),
"/configs/downconfig1");
Review Comment:
temp directories are always absolute. Boy developers *love* calling
toAbsolutePath for no reason :laugh-cry:
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java:
##########
@@ -490,7 +523,7 @@ public void testGet() throws Exception {
final String standardOutput2 =
byteStream2.toString(StandardCharsets.UTF_8);
assertTrue(standardOutput2.startsWith("Copying from 'zk:/getNode'"));
- byte[] fileBytes =
Files.readAllBytes(Paths.get(localFile.getAbsolutePath()));
+ byte[] fileBytes = Files.readAllBytes(localFile.toAbsolutePath());
Review Comment:
why call toAbsolutePath? there's no point; we're going to read it not show
it or pass it off somewhere. I observe many developers love calling
getAbsolutePath but I don't get the point.
##########
solr/core/src/test/org/apache/solr/cli/ZkSubcommandsTest.java:
##########
@@ -398,66 +411,82 @@ public void testUpConfigLinkConfigClearZk() throws
Exception {
assertEquals(confsetname, collectionProps.getStr("configName"));
// test down config
- File configSetDir =
- new File(
- tmpDir, "solrtest-confdropspot-" + this.getClass().getName() + "-"
+ System.nanoTime());
- assertFalse(configSetDir.exists());
+ Path configSetDir =
+ tmpDir.resolve(
+ "solrtest-confdropspot-" + this.getClass().getName() + "-" +
System.nanoTime());
+ assertFalse(Files.exists(configSetDir));
args =
new String[] {
"downconfig",
"--conf-name",
confsetname,
"--conf-dir",
- configSetDir.getAbsolutePath(),
+ configSetDir.toAbsolutePath().toString(),
"-z",
zkServer.getZkAddress()
};
ConfigSetDownloadTool configSetDownloadTool = new ConfigSetDownloadTool();
assertEquals(0, runTool(args, configSetDownloadTool));
- confDir = new File(configSetDir, "conf");
- files = confDir.listFiles();
- zkFiles =
- zkClient.getChildren(ZkConfigSetService.CONFIGS_ZKNODE + "/" +
confsetname, null, true);
- assertEquals(
- "Comparing original conf files that were to be uploadedto what is in
ZK",
- files.length,
- zkFiles.size());
- assertEquals("Comparing downloaded files to what is in ZK", files.length,
zkFiles.size());
+ Path confSetDir = configSetDir.resolve("conf");
+ try (Stream<Path> filesStream = Files.list(confSetDir)) {
+ List<Path> files = filesStream.toList();
+ zkFiles =
+ zkClient.getChildren(ZkConfigSetService.CONFIGS_ZKNODE + "/" +
confsetname, null, true);
+ assertEquals(
+ "Comparing original conf files that were to be uploaded to what is
in ZK",
+ files.size(),
+ zkFiles.size());
+ assertEquals("Comparing downloaded files to what is in ZK",
files.size(), zkFiles.size());
+ }
- File sourceConfDir = new File(ExternalPaths.TECHPRODUCTS_CONFIGSET);
+ Path sourceConfDir = ExternalPaths.TECHPRODUCTS_CONFIGSET;
// filter out all directories starting with . (e.g. .svn)
- Collection<File> sourceFiles =
- FileUtils.listFiles(
- sourceConfDir, TrueFileFilter.INSTANCE, new
RegexFileFilter("[^\\.].*"));
- for (File sourceFile : sourceFiles) {
- int indexOfRelativePath =
- sourceFile
- .getAbsolutePath()
- .lastIndexOf("sample_techproducts_configs" + File.separator +
"conf");
- String relativePathofFile =
- sourceFile
- .getAbsolutePath()
- .substring(indexOfRelativePath + 33,
sourceFile.getAbsolutePath().length());
- File downloadedFile = new File(confDir, relativePathofFile);
- if
(ConfigSetService.UPLOAD_FILENAME_EXCLUDE_PATTERN.matcher(relativePathofFile).matches())
{
- assertFalse(
- sourceFile.getAbsolutePath()
- + " exists in ZK, downloaded:"
- + downloadedFile.getAbsolutePath(),
- downloadedFile.exists());
- } else {
- assertTrue(
- downloadedFile.getAbsolutePath()
- + " does not exist source:"
- + sourceFile.getAbsolutePath(),
- downloadedFile.exists());
- assertTrue(
- relativePathofFile + " content changed",
- FileUtils.contentEquals(sourceFile, downloadedFile));
- }
+ 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
+ .toAbsolutePath()
+ .toString()
+ .lastIndexOf(
+ "sample_techproducts_configs"
+ + FileSystems.getDefault().getSeparator()
Review Comment:
Wherever we see `FileSystems.getDefault()` yet a Path var is in scope (here
it's `sourceFile`), we should use `pathVar.getFileSystem()` instead.
--
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
