Review code

1 files changed, 12 insertions, 1 deletions

diff --git a/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java b/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java
index 64a73bda..d366fefc 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java
@@ -62,9 +62,11 @@ public class InitialSearchTaskFirstTest {
   @InjectMocks
   private InitialSearchTask task;
 
+  // NOTE: Is defined as @Mock, but also manually mocked in "testNode100a" etc -- why?
   @Mock
   private IZmrClient zmrClient;
 
+  // NOTE: Is defined as @Mock, but also manually mocked in "testNode100a" etc -- why?
   @Mock
   private IErnbClient ernbClient;
 
@@ -92,7 +94,7 @@ public class InitialSearchTaskFirstTest {
    */
   @Before
   public void setUp() throws URISyntaxException, EaafStorageException {
-
+    // NOTE: PowerMockito should not be needed, as we don't want to test static and private methods
     task = PowerMockito.spy(task);
 
     httpReq = new MockHttpServletRequest("POST", "https://localhost/authhandler");
@@ -112,6 +114,7 @@ public class InitialSearchTaskFirstTest {
   /**
    * One match, but register update needed
    */
+  // NOTE: Why is the method named "testNode100a"?
   public void testNode100a() throws Exception {
 
     //Mock ZMR
@@ -119,6 +122,9 @@ public class InitialSearchTaskFirstTest {
     String randomBpk = RandomStringUtils.randomNumeric(6);
     zmrResult.add(new RegisterResult(randomBpk,"de/st/max123", "Max_new", "Mustermann", "2011-01-01"));
 
+    // NOTE: Are we using Mockito or these fixed strings in DummyZmrClient?
+    // NOTE: Please mock an interface, not a concrete class
+    // NOTE: But DummyZmrClient is also defined as a bean "ZmrClientForeIDAS" in "eidas_v2_auth.beans.xml"?
     zmrClient = Mockito.mock(DummyZmrClient.class);
     Mockito.when(zmrClient.searchWithPersonIdentifer("max123")).thenReturn(zmrResult);//"de/st/max123"???
     task.setZmrClient(zmrClient);
@@ -137,11 +143,13 @@ public class InitialSearchTaskFirstTest {
       Assert.assertTrue("Wrong bpk", bPk.equals(randomBpk));
 
     } catch (final TaskExecutionException e) {
+      // NOTE: assertTrue is probably the wrong method to use ... why catch the exception anyway?
       Assert.assertTrue("Wrong workflow, should not reach this point", false);
     }
   }
 
   @Test
+  // NOTE: Why is @DirtiesContext after each test necessary? What is changed in the context and why?
   @DirtiesContext
   /**
    * One match, but register update needed
@@ -563,6 +571,7 @@ public class InitialSearchTaskFirstTest {
 
       String bPk = (String)
           pendingReq.getSessionData(AuthProcessDataWrapper.class).getGenericDataFromSession(Constants.DATA_RESULT_MATCHING_BPK);
+      // NOTE: Why "105"? Extract in a constant
       Assert.assertTrue("Wrong bpk", bPk.equals("105"));
     } catch (final TaskExecutionException e) {
       Assert.assertTrue("Wrong workflow, should not reach this point", false);
@@ -578,6 +587,8 @@ public class InitialSearchTaskFirstTest {
 
   @NotNull
   private AuthenticationResponse buildDummyAuthResponseMaxMustermann() throws URISyntaxException {
+    // NOTE: Those strings "de/st/max123" seem to be somehow relevant, but where do we need to use that exact string again?
+    // NOTE: If not, why not using random strings?
     return buildDummyAuthResponse("Max", "Mustermann",
         "de/st/max123", "2011-01-01");
   }