You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "SwingGuy1024 (via GitHub)" <gi...@apache.org> on 2023/04/19 05:27:57 UTC
[GitHub] [commons-io] SwingGuy1024 opened a new pull request, #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
SwingGuy1024 opened a new pull request, #450:
URL: https://github.com/apache/commons-io/pull/450
This should be pulled after IO-789
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory merged pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #450:
URL: https://github.com/apache/commons-io/pull/450
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #450:
URL: https://github.com/apache/commons-io/pull/450#issuecomment-1514557695
You really should not have your address and phone number in emails IMO.
Gary
On Wed, Apr 19, 2023, 06:47 Miguel Muñoz ***@***.***> wrote:
> Gary,
> The PR for IO-790 has changes from both PRs, so that's what you should
> review. Sorry about that. I shouldn't have created two separate JIRA issues
> for this.
> — Miguel Muñoz
> ———
> 4210 Via Arbolada #226Los Angeles, CA 90042
> 323-225-7285
> –
>
> The Sun, with all those planets going around it and dependent on it, can
> still ripen a vine of grapes like it has nothing else to do in the world.
> — Galileo
>
> On Wednesday, April 19, 2023 at 03:07:29 AM PDT, Gary Gregory ***@***.***>
> wrote:
>
>
>
>
> @SwingGuy1024
> You have 2 similar PRs for this class and this one fails. Which PR do you
> want us to review? A PR for a fix should contain changes to main and test,
> where a test fails if the changes to main are not applied.
>
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> You are receiving this because you were mentioned.Message ID: ***@***.***>
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/commons-io/pull/450#issuecomment-1514521020>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAJB6N3E3JB3KHAQCIM5TTDXB67ELANCNFSM6AAAAAAXDRKF3A>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #450: IO-790 Fix symbolic link file filter
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171221533
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
Review Comment:
@SwingGuy1024
Why is `Files.deleteIfExists()` not used here?
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] SwingGuy1024 commented on a diff in pull request #450: IO-790 Fix symbolic link file filter
Posted by "SwingGuy1024 (via GitHub)" <gi...@apache.org>.
SwingGuy1024 commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171833260
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ private static Path createMockSymbolicLink(Path lnk, Path tgt) {
+ try {
+ return Files.createFile(lnk);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ // Mock filter for testing on Windows.
+ private static SymbolicLinkFileFilter createMockFilter() {
+ return new SymbolicLinkFileFilter() {
+ @Override
+ boolean isSymbolicLink(final Path filePath) {
+ return filePath.toFile().exists() && filePath.toString().contains("Link"); // Mock test
+ }
+ };
+ }
+
+ /**
+ * <p>Unit test setup creates a hard file, a symbolic link to the hard file, a hard directory,
+ * and a symbolic link to that directory. All are created in the temp directory</p>
+ * <p>Unit test teardown deletes all four of these files.</p>
+ *
+ * @throws IOException If it fails to create the temporary files
+ */
+ @BeforeAll
+ static void testSetup() throws IOException {
+ final BiFunction<Path, Path, Path> symbolicLinkCreator;
+
+ // We can't create symbolic links on Windows without admin privileges,
+ // so iff that's our OS, we mock them.
+ final String os = System.getProperty("os.name");
+ if (os.toLowerCase().contains("windows")) {
Review Comment:
Done. This is an improvement. Thanks.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] SwingGuy1024 commented on a diff in pull request #450: IO-790 Fix symbolic link file filter
Posted by "SwingGuy1024 (via GitHub)" <gi...@apache.org>.
SwingGuy1024 commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171832215
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
Review Comment:
Because I'm new to the Files API. :) I changed it.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] SwingGuy1024 commented on pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "SwingGuy1024 (via GitHub)" <gi...@apache.org>.
SwingGuy1024 commented on PR #450:
URL: https://github.com/apache/commons-io/pull/450#issuecomment-1514521020
Gary,
The PR for IO-790 has changes from both PRs, so that's what you should review. Sorry about that. I shouldn't have created two separate JIRA issues for this.
— Miguel Muñoz
———
4210 Via Arbolada #226Los Angeles, CA 90042
323-225-7285
–
The Sun, with all those planets going around it and dependent on it, can still ripen a vine of grapes like it has nothing else to do in the world.
— Galileo
On Wednesday, April 19, 2023 at 03:07:29 AM PDT, Gary Gregory ***@***.***> wrote:
@SwingGuy1024
You have 2 similar PRs for this class and this one fails. Which PR do you want us to review? A PR for a fix should contain changes to main and test, where a test fails if the changes to main are not applied.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171196956
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
Review Comment:
Don't change the order of static imports, keep them first.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #450: IO-790 Fix symbolic link file filter
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171219812
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ private static Path createMockSymbolicLink(Path lnk, Path tgt) {
Review Comment:
No weird abbreviations needed.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] SwingGuy1024 commented on a diff in pull request #450: IO-790 Fix symbolic link file filter
Posted by "SwingGuy1024 (via GitHub)" <gi...@apache.org>.
SwingGuy1024 commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171826362
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ private static Path createMockSymbolicLink(Path lnk, Path tgt) {
+ try {
+ return Files.createFile(lnk);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
Review Comment:
Done.
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
Review Comment:
Not sure why this is important in a unit test, for an exception we never expect to see. But I've rewritten the code to let the exception percolate up, as requested.
I feel wrapping a checked exception that we never expect to see is a very efficient way to handle them in interface methods that don't declare exceptions like BiFunction. (If we expect the exception to occur in a real-world scenario, I would handle it differently.)
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #450:
URL: https://github.com/apache/commons-io/pull/450#issuecomment-1514471487
@SwingGuy1024
You have 2 similar PRs for this class and this one fails. Which PR do you want us to review? A PR for a fix should contain changes to main and test, where a test fails if the changes to main are not applied.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] SwingGuy1024 commented on a diff in pull request #450: IO-790 Fix symbolic link file filter
Posted by "SwingGuy1024 (via GitHub)" <gi...@apache.org>.
SwingGuy1024 commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171835409
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
Review Comment:
Done. CheckStyles doesn't have a test for this? (I'm not too familiar with CheckStyles anymore. It's been years.)
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171206899
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
Review Comment:
Let the exception percolate up.
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ private static Path createMockSymbolicLink(Path lnk, Path tgt) {
+ try {
+ return Files.createFile(lnk);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
Review Comment:
Let the exception percolate up.
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ private static Path createMockSymbolicLink(Path lnk, Path tgt) {
+ try {
+ return Files.createFile(lnk);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ // Mock filter for testing on Windows.
+ private static SymbolicLinkFileFilter createMockFilter() {
+ return new SymbolicLinkFileFilter() {
+ @Override
+ boolean isSymbolicLink(final Path filePath) {
+ return filePath.toFile().exists() && filePath.toString().contains("Link"); // Mock test
+ }
+ };
+ }
+
+ /**
+ * <p>Unit test setup creates a hard file, a symbolic link to the hard file, a hard directory,
+ * and a symbolic link to that directory. All are created in the temp directory</p>
+ * <p>Unit test teardown deletes all four of these files.</p>
+ *
+ * @throws IOException If it fails to create the temporary files
+ */
+ @BeforeAll
+ static void testSetup() throws IOException {
+ final BiFunction<Path, Path, Path> symbolicLinkCreator;
+
+ // We can't create symbolic links on Windows without admin privileges,
+ // so iff that's our OS, we mock them.
+ final String os = System.getProperty("os.name");
+ if (os.toLowerCase().contains("windows")) {
Review Comment:
Reuse `org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS`
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #450:
URL: https://github.com/apache/commons-io/pull/450#issuecomment-1514579989
@SwingGuy1024
Thank you for your PR. Noet my comments for future reference. I'll merge and adjust.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] codecov-commenter commented on pull request #450: IO-790 symbolic link file filter test incomplete, needs to handle windows issues
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #450:
URL: https://github.com/apache/commons-io/pull/450#issuecomment-1514208909
## [Codecov](https://codecov.io/gh/apache/commons-io/pull/450?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#450](https://codecov.io/gh/apache/commons-io/pull/450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cf84a34) into [master](https://codecov.io/gh/apache/commons-io/commit/22f6525588afe563a895d6c7a70e03ede86610d1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (22f6525) will **decrease** coverage by `0.08%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #450 +/- ##
============================================
- Coverage 84.62% 84.54% -0.08%
+ Complexity 3291 3288 -3
============================================
Files 223 223
Lines 7913 7914 +1
Branches 946 946
============================================
- Hits 6696 6691 -5
- Misses 970 972 +2
- Partials 247 251 +4
```
| [Impacted Files](https://codecov.io/gh/apache/commons-io/pull/450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../commons/io/filefilter/SymbolicLinkFileFilter.java](https://codecov.io/gh/apache/commons-io/pull/450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vZmlsZWZpbHRlci9TeW1ib2xpY0xpbmtGaWxlRmlsdGVyLmphdmE=) | `100.00% <100.00%> (+14.28%)` | :arrow_up: |
... and [2 files with indirect coverage changes](https://codecov.io/gh/apache/commons-io/pull/450/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] SwingGuy1024 commented on a diff in pull request #450: IO-790 Fix symbolic link file filter
Posted by "SwingGuy1024 (via GitHub)" <gi...@apache.org>.
SwingGuy1024 commented on code in PR #450:
URL: https://github.com/apache/commons-io/pull/450#discussion_r1171832585
##########
src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java:
##########
@@ -17,21 +17,198 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+import java.io.File;
+import java.io.IOException;
import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.function.BiFunction;
import org.apache.commons.io.file.PathUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
+ public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
+ public static final String MISSING = "Missing";
+ private static File testTargetFile; // hard file
+ private static Path testTargetPath; // hard file Path
+ private static File parentDirectoryFile; // System Temp directory
+ private static File testLinkFile; // symbolic link to hard file
+ private static String linkName; // Name of link file
+ private static Path testLinkPath; // symbolic link to hard file Path
+ private static File targetDirFile; //
+ private static Path targetDirPath; // hard directory Path
+ private static Path testLinkDirPath; // symbolic link to hardDirectory
+ private static File testLinkDirFile;
+ private static File missingFile; // non-existent file
+ private static SymbolicLinkFileFilter filter;
+
+ private static Path createRealSymbolicLink(Path link, Path target) {
+ try {
+ if (Files.exists(link)) {
+ Files.delete(link);
+ }
+ return Files.createSymbolicLink(link, target);
+ } catch (IOException e) {
+ throw new IllegalStateException("Failure to create Symbolic Link", e);
+ }
+ }
+
+ private static Path createMockSymbolicLink(Path lnk, Path tgt) {
Review Comment:
Done.
--
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-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org