rombert commented on code in PR #5:
URL:
https://github.com/apache/sling-org-apache-sling-commons-mime/pull/5#discussion_r1008227094
##########
src/test/java/org/apache/sling/commons/mime/internal/MimeTypeServiceImplTest.java:
##########
@@ -43,6 +44,18 @@ public class MimeTypeServiceImplTest extends TestCase {
private static final String TEXT_PLAIN = "text/plain";
+ private static final String EPS = "eps";
+
+ private static final Map<String, String> epsMimeTypeExt = new HashMap<>();
+
+ static {
+ epsMimeTypeExt.put("application/eps", EPS);
Review Comment:
I think it's enough to test two different mime types, right? Otherwise we
overspecify the behaviour. And for two tests you can use two assertions instead
of a map and a loop in the test.
##########
src/test/java/org/apache/sling/commons/mime/internal/MimeTypeServiceImplTest.java:
##########
@@ -152,6 +165,29 @@ public void testProvider2() throws Exception {
assertNull(this.service.getMimeType(GIF));
}
+ public void testMultipleMimeTypes() throws Exception {
+ loadMimeTypes(MimeTypeServiceImpl.CORE_MIME_TYPES);
+ loadMimeTypes(MimeTypeServiceImpl.MIME_TYPES);
+
+ for (String mm : epsMimeTypeExt.keySet()) {
+ assertEquals("Extension " + mm + " (1)", epsMimeTypeExt.get(mm),
this.service.getExtension(mm));
Review Comment:
The name _mm_ is a bit opaque to me, can you please use something to
understand?
##########
src/test/java/org/apache/sling/commons/mime/internal/MimeTypeServiceImplTest.java:
##########
@@ -152,6 +165,29 @@ public void testProvider2() throws Exception {
assertNull(this.service.getMimeType(GIF));
}
+ public void testMultipleMimeTypes() throws Exception {
+ loadMimeTypes(MimeTypeServiceImpl.CORE_MIME_TYPES);
+ loadMimeTypes(MimeTypeServiceImpl.MIME_TYPES);
+
+ for (String mm : epsMimeTypeExt.keySet()) {
+ assertEquals("Extension " + mm + " (1)", epsMimeTypeExt.get(mm),
this.service.getExtension(mm));
+ }
+ }
+
+ public void testNegativeTests() throws Exception {
+ loadMimeTypes(MimeTypeServiceImpl.CORE_MIME_TYPES);
+ loadMimeTypes(MimeTypeServiceImpl.MIME_TYPES);
+
+ // null
+ String mm = null;
+ assertEquals("Extension " + mm, mm, this.service.getExtension(mm));
+ this.service.registerMimeType("application/invalid-1", mm);
+ mm = "";
+ assertEquals("Extension " + mm, null, this.service.getExtension(mm));
+ this.service.registerMimeType("application/invalid-1", mm);
Review Comment:
Why are these two registration methods needed? It is unclear what the test
is validating, since you have assertions and then apparently an expected
exception.
##########
src/test/java/org/apache/sling/commons/mime/internal/MimeTypeServiceImplTest.java:
##########
@@ -152,6 +165,29 @@ public void testProvider2() throws Exception {
assertNull(this.service.getMimeType(GIF));
}
+ public void testMultipleMimeTypes() throws Exception {
+ loadMimeTypes(MimeTypeServiceImpl.CORE_MIME_TYPES);
+ loadMimeTypes(MimeTypeServiceImpl.MIME_TYPES);
+
+ for (String mm : epsMimeTypeExt.keySet()) {
+ assertEquals("Extension " + mm + " (1)", epsMimeTypeExt.get(mm),
this.service.getExtension(mm));
+ }
+ }
+
+ public void testNegativeTests() throws Exception {
+ loadMimeTypes(MimeTypeServiceImpl.CORE_MIME_TYPES);
+ loadMimeTypes(MimeTypeServiceImpl.MIME_TYPES);
+
+ // null
+ String mm = null;
+ assertEquals("Extension " + mm, mm, this.service.getExtension(mm));
Review Comment:
I find it hard to understand what this test does. Perhaps split into
multiple methods that each validate a "negative" path?
##########
src/main/java/org/apache/sling/commons/mime/internal/MimeTypeServiceImpl.java:
##########
@@ -166,9 +166,17 @@ public void registerMimeType(String mimeType, String...
extensions) {
}
}
+ addToExtensions(defaultExtension, mimeType, extensions);
+ }
+ private void addToExtensions(String defaultExtension, String mimeType,
String[] extensions) {
Review Comment:
I think the code would be easier to follow if you cleaned up the extensions
array in the `registerMimeType` method ( lowercase, skip null/empty entries )
and no longer had to do so in `addToExtensions`.
I also think it's a good place to use `putIfAbsent` to simplify the code.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
