Update TemporaryFolder.newFolder(String) to support passing in paths#1402
Conversation
| if (folder.startsWith(File.separator)) { | ||
| throw new IOException("Folder name cannot start with a file separator"); | ||
| } | ||
| return newFolder(folder.split(FILE_SEPARATOR_REGEX)); |
There was a problem hiding this comment.
Instead of the regex, can we use new File(folder) and call getParent() until it returns null? This way a test could use newFolder("foo/bar") and be able to run on Windows and Unix.
There was a problem hiding this comment.
I didn't realize that File supported / as a path separator for Windows.
Instead of your solution, how about I rewite this method to use mkdirs()?
There was a problem hiding this comment.
How would you report already existing intermediate folders (see newFolder(String[])) that way?
There was a problem hiding this comment.
newFolder(String...) does not report already existing intermediate directories
There was a problem hiding this comment.
From the code it looks like it does… 😯
There was a problem hiding this comment.
Changed to use mkdirs() and added a test for paths that contain a forward slash and for paths that contain the file separator.
| tempFolder.newFolder("temp1/temp2"); | ||
| File temp1 = new File(tempFolder.getRoot(), "temp1"); | ||
| assertFileIsDirectory(temp1); | ||
| assertFileIsDirectory(new File(temp1, "temp2")); |
There was a problem hiding this comment.
This test won't run on Windows, right?
|
BTW, the behavior changes in 4.12 broke some of our tests. When I am back in the office tomorrow I will verify that this change resolves the problems. |
kcooney
force-pushed
the
TemporaryFolder-support-path-separator
branch
from
370cae0 to
b94b56e
Compare
|
PTAL. Thanks, @marcphilipp ! |
…ashes. In JUnit 4.11, newFolder(String) was changed to no longer support paths with a file separator. This has been fixed. The overload of newFolder() that supports passing in multiple strings still does not allow strings containing file separators.
kcooney
force-pushed
the
TemporaryFolder-support-path-separator
branch
from
b94b56e to
c47bb64
Compare
|
I verified that this is a regression from the behavior in JUnit 4.11 and changed the commit comment accordingly. I also removed the use of Any objections to merging this? Note I'm also considering changing newFolder(String...) to support paths with slashes, since the inconsistency is weird and supporting slashes would be consistent with |
| * Returns a new fresh folder with the given path under the temporary | ||
| * folder. | ||
| */ | ||
| public File newFolder(String folder) throws IOException { |
There was a problem hiding this comment.
We should probably rename folder to path, right?
| File file = new File(getRoot(), folder); | ||
| if (!file.mkdirs()) { | ||
| throw new IOException( | ||
| "a folder with the name \'" + folder + "\' already exists"); |
There was a problem hiding this comment.
Is this error message really accurate? I know it's the same as in the other method but I'm sure there are other reasons why a folder could not be created.
👍
None at all.
I think supporting slashes is great! I've tried to find out why it was implemented that way. From #946 it seems 4.10 used only |
|
Please add a description of this improvement to the draft of the release notes. |
|
@marcphilipp I updated the release notes. Sorry, I somehow didn't see some of your comments. I'll address them soon. |
|
Thanks! And no worries about those comments. 🙂 |
…ashes. (junit-team#1402) In JUnit 4.11, newFolder(String) was changed to no longer support paths with a file separator. This has been fixed. The overload of newFolder() that supports passing in multiple strings still does not allow strings containing file separators.
…ashes. (junit-team#1402) In JUnit 4.11, newFolder(String) was changed to no longer support paths with a file separator. This has been fixed. The overload of newFolder() that supports passing in multiple strings still does not allow strings containing file separators.
2 participants
that include path separator characters. The overload of newFolder() that
supports passing in multiple strings still does not allow path separators.