[GitHub] [knox] zeroflag commented on a diff in pull request #597: KNOX-2762 Bug fixes for spaces around delimiters with all reviewed comments addressed

[GitBox <gi...@apache.org>]

zeroflag commented on code in PR #597:
URL: https://github.com/apache/knox/pull/597#discussion_r905386669


##########
gateway-provider-security-authz-composite/src/main/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzDeploymentContributor.java:
##########
@@ -71,7 +71,7 @@ public void contributeFilter( DeploymentContext context, Provider provider, Serv
   }
 
   List parseProviderNames(String providerNames) {

Review Comment:
   What's the reason for not having a type parameter for the List? (E.g. List<String>)



##########
gateway-provider-security-authz-composite/src/test/java/org/apache/knox/gateway/deploy/impl/CompositeAuthzProviderTest.java:
##########
@@ -83,13 +85,19 @@ public void testingNullandEmptyProviderNames() throws Exception {
     String testnames = "";
     CompositeAuthzDeploymentContributor c = new CompositeAuthzDeploymentContributor();
     List providerNames = c.parseProviderNames(testnames);
-    assertEquals(providerNames.size(), 1);
-    assertEquals(providerNames.get(0), "");
+    assertSame(providerNames.size(),0);

Review Comment:
   Here the order of parameters are reversed. The first is the expected and the second is the actual. 
   This is not a correctness issue, but the error message will misleading if the test fails.
   
   For example if providerNames has a size of 3 it'll say: expected 3 got 0, but it is the other way around.
   
   More importantly what's the reason for using assertSame instead of assertEquals? My understanding is that assertSame checks identity (`==`) not equality (`.equals`).
   



-- 
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: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org