Update TemporaryFolder.newFolder() to give a better error message if a path contains a slash #974
Conversation
| * @param folderName Name of the folder being created | ||
| * @throws IOException | ||
| */ | ||
| private void validateFolderName(String folderName) throws IOException { |
|
Thanks! Could you add some tests? |
|
Will fix the indentation and will remove the null check. I am planning to add the following test cases in TemporaryFolderUsageTest class-
Please let me know if I am missing out on other important test cases. |
|
In the latest fix, I fixed indentation and also removed the null check. Have also added two additional tests. Please let me know your comments on this. |
| thrown.expect(IOException.class); | ||
| String errorMsg = "Folder name cannot consist of multiple path components separated by a file separator." | ||
| + " Please use newFolder('MyParentFolder','MyFolder') to create hierarchies of folders"; | ||
| thrown.expectMessage(errorMsg); |
There was a problem hiding this comment.
note that expectMessage does a substring match, so you don't need to put the entire error message. I would rather not hard-code the entire error message in the test because it's unnecessary brittle. Could you just check for a substring like "name cannot consist of multiple path components"? Thanks
kcooney
changed the title
|
I updated the description to state what the change is. Thanks for adding the tests. After making the requested changes, could you squash the commits? See https://github.com/junit-team/junit/wiki/Pull-Requests for details. I'm not sure we will get this into 4.12. I want to make sure that the people discussing this bug agree on this as the fix. |
|
Done. I am waiting to hear from other reviewers. What is the deadline for 4.12? |
|
LGTM! |
|
@priav03 Thanks. We already sent out the first release candidate for 4.12. I'm not sure what kinds of changes the other JUnit owners want to allow in before the final 4.12 release |
| * | ||
| * @param folderName | ||
| * Name of the folder being created | ||
| * @throws IOException |
There was a problem hiding this comment.
The @throws is not necessary and could be removed.
There was a problem hiding this comment.
Thanks for the explanation. I realize the @throws is indeed not necessary. I have removed the throws as suggested by @stefanbirkner .
|
I'm fine with adding this pull request to JUnit 4.12 if it is ready. |
|
Looks fine to me as it gets rid of the misleading error message. |
… error message if a path contains a slash
|
+1 for releasing this in 4.12 if it's ready. |
|
I have incorporated the changes suggested by @stefanbirkner . |
|
Hi all. Did we decide on including/excluding this fix for 4.12? |
Update TemporaryFolder.newFolder() to give a better error message if a path contains a slash
|
@priav03 Thanks again for the fix. Could you update the release notes on the wiki? |
|
The release notes have already been moved to the repo to prepare for You can post them as a comment right here and I can add them. |
|
Sure. I have used @paulduffin 's description of the issue to state the behavior prior to this fix. Here is the content for release notes - Pull request #974 - Updated TemporaryFolder.newFolder() to give an error message if a path contains a slash. If you call TemporaryFolder.newFolder("foo/bar") in JUnit 4.10 the method returns a File object for the new folder but actually fails to create it. That is contrary to the expected behaviour of the method which is to actually create the folder. In JUnit 4.11 the same call throws an exception. Nowhere in the documentation does it explain that the String(s) passed to that method can only be single path components. With this fix, folder names are validated to contain single path name. If the folder name consists of multiple path names, an exception is thrown stating that usage of multiple path components in a string containing folder name is disallowed. |
|
Please let me know if I need to make any changes to the content posted above. |
|
Looks fine to me.
|
Please review the fix and let me know if it does good.