-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDFS-17249. Fix TestDFSUtil.testIsValidName() unit test failure #6249
Conversation
💔 -1 overall
This message was automatically generated. |
@GauthamBanasandra , hello sir,do you have time to review it,Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please enhance the unit test testIsValidName
with the following -
if (Shell.WINDOWS) {
assertTrue(DFSUtil.isValidName("/C:/some/path"));
} else {
assertFalse(DFSUtil.isValidName("/C:/some/path"));
}
@@ -661,9 +661,12 @@ public static boolean isValidName(String src) { | |||
String[] components = StringUtils.split(src, '/'); | |||
for (int i = 0; i < components.length; i++) { | |||
String element = components[i]; | |||
// For Windows, we must allow the : in the drive letter. | |||
if (Shell.WINDOWS && i == 1 && element.contains(":")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please make an endsWith
check for :
instead of element.contains
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiuGuH could you please file a new bug on issues.apache.org and use it instead of tagging to HDFS-17248
?
Sure. I can. But what's the difference between them that I need to notice? Thanks |
@LiuGuH in the Hadoop community we normally follow the convention where the bug title in JIRA matches that of the Github PR. In this case, HDFS-17248 in JIRA has the title Thus, you'll need to create a new JIRA for this. |
@GauthamBanasandra I create a new JIRA for this. If the title is not matches, please give me a suggestion .Thanks. |
🎊 +1 overall
This message was automatically generated. |
@LiuGuH I apologize for the confusion. The JIRA that you had created originally (HDFS-17248) had no issues. I overlooked the number and thought it was the same as HDFS-17246. Anyway, we can go ahead with what you've tagged currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me. I've started a Windows CI run to make sure we don't break anything on Windows - https://ci-hadoop.apache.org/view/Hadoop/job/hadoop-qbt-trunk-java8-win10-x86_64/306/console
We can merge once the CI run finishes. It'll take around 1.5 days to finish.
Seems like the run was aborted due to a restart -
I've started a new run - https://ci-hadoop.apache.org/view/Hadoop/job/hadoop-qbt-trunk-java8-win10-x86_64/310/console |
having hit this myself, I'm going to say "this PR needss to include the name passed in rather than just assert true/false" proposed, assertValidName(String name) {
assertFalse("Should have been rejected '" + name + "'", isValidName(name);
} ...and all asserts in the test case switched over to this. think how much easier it will be debugging yetus falures the next time there's a regressoin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proposed changes to the test case to remove duplication and assist in analysing future test failures. It is always good to clean up tests when the opportunity arises, with my ideal "you can understand what the assertion failure was without trying to correlate the line in your IDE"
When this run finished , I will add all asserts in the test case switched over to this @steveloughran Thanks assertValidName(String name) { |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@steveloughran could you please review this PR? |
…he#6249) Contributed by liuguanghua.
Description of PR
TestDFSUtil.testIsValidName runs failed when assertFalse(DFSUtil.isValidName("/foo/:/bar")) , fixed it.
HDFS-17249.
How was this patch tested?
Add test case in TestDFSUtil.testIsValidName.