steveloughran commented on code in PR #6546: URL: https://github.com/apache/hadoop/pull/6546#discussion_r1537853951
########## hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java: ########## @@ -79,6 +79,16 @@ public class StringUtils { public static final Pattern ENV_VAR_PATTERN = Shell.WINDOWS ? WIN_ENV_VAR_PATTERN : SHELL_ENV_VAR_PATTERN; + /** + * {@link #getTrimmedStringCollectionSplitByEquals(String)} throws + * {@link IllegalArgumentException} with error message starting with this string + * if the argument provided is not valid representation of non-empty key-value + * pairs. Review Comment: tag as @VisibleForTesting, add a {@value} element to the javadocs ########## hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java: ########## @@ -790,6 +790,84 @@ public void testStringCollectionSplitByEquals() { .containsEntry("element.xyz.key5", "element.abc.val5") .containsEntry("element.xyz.key6", "element.abc.val6") .containsEntry("element.xyz.key7", "element.abc.val7"); + + configuration.set("custom_key", + "element.first.key1 = element.first.val2 ,element.first.key1 =element.first.val1"); + splitMap = + S3AUtils.getTrimmedStringCollectionSplitByEquals( + configuration, "custom_key"); + Assertions + .assertThat(splitMap) + .describedAs("Map of key value pairs split by equals(=) and comma(,)") + .hasSize(1) + .containsEntry("element.first.key1", "element.first.val1"); + + configuration.set("custom_key", + ",,, , ,, ,element.first.key1 = element.first.val2 ," + + "element.first.key1 = element.first.val1 , ,,, ,"); + splitMap = S3AUtils.getTrimmedStringCollectionSplitByEquals( + configuration, "custom_key"); + Assertions + .assertThat(splitMap) + .describedAs("Map of key value pairs split by equals(=) and comma(,)") + .hasSize(1) + .containsEntry("element.first.key1", "element.first.val1"); + + configuration.set("custom_key", ",, , , ,, ,"); + splitMap = S3AUtils.getTrimmedStringCollectionSplitByEquals( + configuration, "custom_key"); + Assertions + .assertThat(splitMap) + .describedAs("Map of key value pairs split by equals(=) and comma(,)") + .hasSize(0); + } + + /** + * Tests for the string utility that will be used by S3A credentials provider. + */ + @Test + public void testStringCollectionSplitByEqualsFailure() throws Exception { Review Comment: this would be better off with a helper method "expectInvalidArgument(value)" and 5 separate test methods. less code, more isolation. -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org