[
https://issues.apache.org/jira/browse/GEODE-8572?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17208991#comment-17208991
]
ASF GitHub Bot commented on GEODE-8572:
---------------------------------------
jchen21 commented on a change in pull request #5595:
URL: https://github.com/apache/geode/pull/5595#discussion_r500475855
##########
File path:
geode-gfsh/src/integrationTest/java/org/apache/geode/management/internal/cli/util/LogExporterIntegrationTest.java
##########
@@ -50,127 +48,146 @@
@Category({GfshTest.class, LoggingTest.class})
public class LogExporterIntegrationTest {
+ @Rule
+ public ServerStarterRule server = new ServerStarterRule();
private final LogFilter filter = new LogFilter(Level.INFO, null, null);
- private LogExporter logExporter;
- private Properties properties;
-
- @Rule
- public ServerStarterRule server = new ServerStarterRule();
+ public Path serverFilesDir;
@Before
- public void before() {
- properties = new Properties();
- // make sure the server's working dir has no log files or stats file to
begin with, since in
- // some tests we are asserting on the # of log files and stats files
created by the server
- File workingDir = server.getWorkingDir();
- Arrays.stream(workingDir.listFiles())
- .filter(f -> (f.getName().endsWith(".log") ||
f.getName().endsWith(".gfs")))
- .forEach(FileUtils::deleteQuietly);
+ public void createServerFilesDir() throws IOException {
+ // Name the directory after this test instance and the Gradle test worker,
to ensure that tests
+ // running in parallel use different directories.
+ String testRunnerID = System.getProperty("org.gradle.test.worker",
"standalone");
+ int testInstanceID = System.identityHashCode(this);
+ String className = getClass().getSimpleName();
+ String dirName = String.format("%s-%x-%s", className, testInstanceID,
testRunnerID);
+ serverFilesDir =
Files.createDirectories(Paths.get(dirName)).normalize().toAbsolutePath();
+ }
+
+ @After
+ public void deleteServerFilesDir() {
+ FileUtils.deleteQuietly(serverFilesDir.toFile());
}
@Test
public void serverStartedWithWrongSuffix() throws Exception {
- properties.setProperty(LOG_FILE, new File("test.txt").getAbsolutePath());
- properties.setProperty(STATISTIC_ARCHIVE_FILE, "archive.archive");
- server.withProperties(properties).startServer();
- File serverWorkingDir = server.getWorkingDir();
-
- logExporter = new LogExporter(filter, new File(serverWorkingDir,
"test.log"),
- new File(serverWorkingDir, "stats.gfs"));
- List<Path> logFiles = logExporter.findLogFiles(serverWorkingDir.toPath());
- assertThat(logFiles).isEmpty();
-
- List<Path> statsFiles =
logExporter.findStatFiles(serverWorkingDir.toPath());
- assertThat(statsFiles).isEmpty();
+ String logFileNameWithWrongSuffix = "test.txt";
+ String statsFileNameWithWrongSuffix = "archive.archive";
+
+ Path logFile = serverFilesDir.resolve(logFileNameWithWrongSuffix);
+ Path statsFile = serverFilesDir.resolve(statsFileNameWithWrongSuffix);
+
+ server.withProperty(LOG_FILE, logFile.toString())
+ .withProperty(STATISTIC_ARCHIVE_FILE, statsFile.toString())
+ .startServer();
+
+ LogExporter logExporter = new LogExporter(filter, null, null);
+ List<Path> logFiles = logExporter.findLogFiles(serverFilesDir);
+
+ assertThat(logFiles)
+ .as("log files")
+ .isEmpty();
+
+ List<Path> statsFiles = logExporter.findStatFiles(serverFilesDir);
+ assertThat(statsFiles)
+ .as("stat files")
+ .isEmpty();
}
@Test
public void serverStartedWithCorrectSuffix() throws Exception {
- // ("relative log file is problematic in the test environment")
- properties.setProperty(LOG_FILE, new File("test.log").getAbsolutePath());
- properties.setProperty(STATISTIC_ARCHIVE_FILE, "archive.gfs");
- server.withProperties(properties).startServer();
- File serverWorkingDir = server.getWorkingDir();
-
- logExporter = new LogExporter(filter, new File(serverWorkingDir,
"test.log"),
- new File(serverWorkingDir, "archive.gfs"));
- List<Path> logFiles = logExporter.findLogFiles(serverWorkingDir.toPath());
- assertThat(logFiles).hasSize(1);
- assertThat(logFiles.get(0)).hasFileName("test.log");
-
- List<Path> statsFiles =
logExporter.findStatFiles(serverWorkingDir.toPath());
- assertThat(statsFiles).hasSize(1);
- assertThat(statsFiles.get(0)).hasFileName("archive.gfs");
+ String logFileName = "test.log";
+ String statsFileName = "archive.gfs";
+ Path logFile = serverFilesDir.resolve(logFileName);
+ Path statsFile = serverFilesDir.resolve(statsFileName);
+
+ server.withProperty(LOG_FILE, logFile.toString())
+ .withProperty(STATISTIC_ARCHIVE_FILE, statsFile.toString())
+ .startServer();
+
+ LogExporter logExporter = new LogExporter(filter, null, null);
+ List<Path> logFiles = logExporter.findLogFiles(serverFilesDir);
+
+ assertThat(logFiles)
+ .as("log files")
+ .hasSize(1);
+ assertThat(logFiles.get(0)).hasFileName(logFileName);
+
+ List<Path> statsFiles = logExporter.findStatFiles(serverFilesDir);
+ assertThat(statsFiles)
+ .as("stat files")
+ .hasSize(1);
+ assertThat(statsFiles.get(0)).hasFileName(statsFileName);
}
@Test
@Ignore("GEODE-2574: fix .gz suffix")
Review comment:
Not sure why this test is @Ignore. Any change to the test is not
executed.
##########
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/util/LogExporter.java
##########
@@ -181,12 +180,13 @@ private long filterAndSize(Path originalLogFile) throws
IOException {
}
private List<Path> findFiles(Path workingDir, Predicate<Path> fileSelector)
throws IOException {
- Stream<Path> selectedFiles/* = null */;
if (!workingDir.toFile().isDirectory()) {
return Collections.emptyList();
}
- selectedFiles =
Files.list(workingDir).filter(fileSelector).filter(this.logFilter::acceptsFile);
-
- return selectedFiles.collect(toList());
+ return Files.list(workingDir)
+ .filter(Files::isRegularFile)
Review comment:
By default, this follows the symbolic link. Just make sure this is
expected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> LogExporter throws if a directory matches its file selector
> -----------------------------------------------------------
>
> Key: GEODE-8572
> URL: https://issues.apache.org/jira/browse/GEODE-8572
> Project: Geode
> Issue Type: Bug
> Components: gfsh
> Affects Versions: 1.13.0
> Reporter: Dale Emery
> Priority: Major
> Labels: GeodeOperationAPI, pull-request-available
>
> {{LogExporter}} tries to read and export any directory entry whose name is
> accepted by its log file selector predicate. The predicate accepts any entry
> whose name, when converted to lower case, contains ".log". If one of those
> entries is directory, the predicate accepts it anyway. When {{LogExporter}}
> attempts to create a {{FileReader}} read it, the {{FileReader}} constructor
> throws {{FileNotFoundException}}.
> There is a stat file selector predicate that works similarly.
> {{LogExporter}}'s log and stat file selector predicates should accept only
> files, and not directories. And perhaps the should accept only files whose
> names _end_ in the appropriate extension, rather than _containing_ the
> characters. The predicates are defined in {{LogExporter}}'s
> {{findLogFiles()}} and {{findStatFiles()}} methods.
> I discovered this defect by running {{LogExporterIntegrationTest}} on macOS.
> Each test's preparation writes some to-be-exported files into a temporary
> directory that, on macOS, may contain numerous other files and directories.
> One of those directories (e.g.
> /var/folders/yz/6y59fxln38d7lf2jxng1zgg40000gn/T/com.apple.LoginUserService),
> which matches the {{LogExporter}}'s predicate.
> These tests should also be changed to write their to-be-exported files to a
> directory that is known to be empty.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
