pzampino commented on code in PR #615:
URL: https://github.com/apache/knox/pull/615#discussion_r939161193
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -48,25 +52,25 @@ private GatewayConfig mockConfig(Set<String>
privilegedUsers, Set<String> nonPri
@Test
- public void userIsInNeitherOfTheGroups() {
+ public void userIsInNeitherOfTheGroups() throws ServiceLifecycleException {
GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")),
new HashSet<>(Arrays.asList("tom", "guest")), 3, 2);
- verifier.init(config);
+ verifier.init(config, options);
for (int i = 0; i < 4; i++) {
Review Comment:
What is the intent of this loop to verify the same user? Is it for testing
the limit(s) defined in the mockConfig? If so, it would be better to reference
the same constant for both the creation of the mockConfig and the subsequent
loop. Some comments might be helpful here as well, even if only a message in
the assertion.
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -48,25 +52,25 @@ private GatewayConfig mockConfig(Set<String>
privilegedUsers, Set<String> nonPri
@Test
- public void userIsInNeitherOfTheGroups() {
+ public void userIsInNeitherOfTheGroups() throws ServiceLifecycleException {
Review Comment:
Generally, in Knox, test methods begin with the prefix "test". While it may
seem unnecessary with the @Test annotation, it is the pattern followed
throughout the codebase.
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -18,22 +18,26 @@
package org.apache.knox.gateway.session.control;
import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
import org.easymock.EasyMock;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import java.util.Arrays;
+import java.util.HashMap;
import java.util.HashSet;
+import java.util.Map;
import java.util.Set;
public class ConcurrentSessionVerifierTest {
-
private ConcurrentSessionVerifier verifier;
+ private Map<String, String> options;
@Before
public void setUp() {
- verifier = ConcurrentSessionVerifier.getInstance();
+ verifier = new ConcurrentSessionVerifier();
+ options = new HashMap<>();
Review Comment:
Does this Map ever get populated? If not, you could replace this with
Collections.emptyMap().
##########
gateway-server/src/test/java/org/apache/knox/gateway/session/control/ConcurrentSessionVerifierTest.java:
##########
@@ -106,25 +110,25 @@ public void userIsNotPrivileged() {
}
@Test
- public void privilegedLimitIsZero() {
+ public void privilegedLimitIsZero() throws ServiceLifecycleException {
GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")),
new HashSet<>(Arrays.asList("tom", "guest")), 0, 2);
- verifier.init(config);
+ verifier.init(config, options);
Assert.assertFalse(verifier.verifySessionForUser("admin"));
}
@Test
- public void nonPrivilegedLimitIsZero() {
+ public void nonPrivilegedLimitIsZero() throws ServiceLifecycleException {
GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")),
new HashSet<>(Arrays.asList("tom", "guest")), 3, 0);
- verifier.init(config);
+ verifier.init(config, options);
Assert.assertFalse(verifier.verifySessionForUser("tom"));
}
@Test
- public void sessionsDoNotGoToNegative() {
+ public void sessionsDoNotGoToNegative() throws ServiceLifecycleException {
GatewayConfig config = mockConfig(new HashSet<>(Arrays.asList("admin")),
new HashSet<>(Arrays.asList("tom", "guest")), 2, 2);
- verifier.init(config);
+ verifier.init(config, options);
Assert.assertNull(verifier.getUserConcurrentSessionCount("admin"));
Review Comment:
Why does this return null instead of zero?
--
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]
