fix random failure of DoPrivilegedTest

unit test is based on ThreadLocal but wasn't isolated
in addition, assertions where actually not verified as the code was written
completed test to make sure the initial session is restored once the DoPriviledge task has run

diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/DoPrivilegedTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/DoPrivilegedTest.java
index 9ca8fb4c95aa2a9d198a3cdc531a7dab777ee69e..1c15f020e079ae1c627592caeeeb7fbcbe2935d7 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/user/DoPrivilegedTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/DoPrivilegedTest.java
@@ -19,46 +19,74 @@
  */
 package org.sonar.server.user;
 
+import org.junit.Before;
 import org.junit.Test;
+import org.sonar.server.tester.MockUserSession;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
 
 public class DoPrivilegedTest {
 
-  ThreadLocalUserSession threadLocalUserSession = new ThreadLocalUserSession();
+  private static final String LOGIN = "dalailaHidou!";
+
+  private ThreadLocalUserSession threadLocalUserSession = new ThreadLocalUserSession();
+  private MockUserSession session = new MockUserSession(LOGIN);
+
+  @Before
+  public void setUp() throws Exception {
+    threadLocalUserSession.set(session);
+  }
 
   @Test
   public void should_allow_everything_in_privileged_block_only() {
-    DoPrivileged.execute(new DoPrivileged.Task(threadLocalUserSession) {
-      @Override
-      protected void doPrivileged() {
-        UserSession userSession = threadLocalUserSession.get();
-        assertThat(userSession.isLoggedIn()).isFalse();
-        assertThat(userSession.hasGlobalPermission("any permission")).isTrue();
-        assertThat(userSession.hasProjectPermission("any permission", "any project")).isTrue();
-      }
-    });
+    UserSessionCatcherTask catcher = new UserSessionCatcherTask();
+
+    DoPrivileged.execute(catcher);
 
-    assertThat(threadLocalUserSession.isLoggedIn()).isFalse();
+    // verify the session used inside Privileged task
+    assertThat(catcher.userSession.isLoggedIn()).isFalse();
+    assertThat(catcher.userSession.hasGlobalPermission("any permission")).isTrue();
+    assertThat(catcher.userSession.hasProjectPermission("any permission", "any project")).isTrue();
+
+    // verify session in place after task is done
+    assertThat(threadLocalUserSession.get()).isSameAs(session);
   }
 
   @Test
   public void should_lose_privileges_on_exception() {
+    UserSessionCatcherTask catcher = new UserSessionCatcherTask() {
+      @Override
+      protected void doPrivileged() {
+        super.doPrivileged();
+        throw new RuntimeException("Test to lose privileges");
+      }
+    };
+
     try {
-      DoPrivileged.execute(new DoPrivileged.Task(threadLocalUserSession) {
-        @Override
-        protected void doPrivileged() {
-          UserSession userSession = threadLocalUserSession.get();
-          assertThat(userSession.isLoggedIn()).isTrue();
-          assertThat(userSession.hasGlobalPermission("any permission")).isTrue();
-          assertThat(userSession.hasProjectPermission("any permission", "any project")).isTrue();
-
-          throw new RuntimeException("Test to lose privileges");
-        }
-      });
-    } catch(Throwable ignored) {
-      assertThat(threadLocalUserSession.isLoggedIn()).isFalse();
+      DoPrivileged.execute(catcher);
+      fail("An exception should have been raised!");
+    } catch (Throwable ignored) {
+      // verify session in place after task is done
+      assertThat(threadLocalUserSession.get()).isSameAs(session);
+
+      // verify the session used inside Privileged task
+      assertThat(catcher.userSession.isLoggedIn()).isFalse();
+      assertThat(catcher.userSession.hasGlobalPermission("any permission")).isTrue();
+      assertThat(catcher.userSession.hasProjectPermission("any permission", "any project")).isTrue();
     }
   }
 
+  private class UserSessionCatcherTask extends DoPrivileged.Task {
+    UserSession userSession;
+
+    public UserSessionCatcherTask() {
+      super(DoPrivilegedTest.this.threadLocalUserSession);
+    }
+
+    @Override
+    protected void doPrivileged() {
+      userSession = threadLocalUserSession.get();
+    }
+  }
 }

diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/RubyUserSessionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/RubyUserSessionTest.java
index d75a7c733b1738e8c051d8012471330eee7c4671..ac33f8d40780131d7b6c55f57f1747abc5bea827 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/user/RubyUserSessionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/RubyUserSessionTest.java
@@ -20,6 +20,7 @@
 package org.sonar.server.user;
 
 import java.util.Locale;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.sonar.core.platform.ComponentContainer;
@@ -38,10 +39,19 @@ public class RubyUserSessionTest {
 
   @Before
   public void setUp() {
+    // for test isolation
+    threadLocalUserSession.remove();
+
     when(platform.getContainer()).thenReturn(componentContainer);
     when(componentContainer.getComponentByType(ThreadLocalUserSession.class)).thenReturn(threadLocalUserSession);
   }
 
+  @After
+  public void tearDown() throws Exception {
+    // clean up for next test
+    threadLocalUserSession.remove();
+  }
+
   @Test
   public void should_set_session() {
     underTest.setSessionImpl(123, "karadoc", "Karadoc", newArrayList("sonar-users"), "fr");

diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java
index 1b69404d8c12bfcda167c444a557373d6166d0b1..0347c0a5c4b7a16a267439355363b25859e1495b 100644 (file)
--- a/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java
@@ -20,7 +20,9 @@
 package org.sonar.server.user;
 
 import java.util.Locale;
+import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.sonar.server.tester.MockUserSession;
@@ -30,11 +32,18 @@ import static org.assertj.core.api.Assertions.assertThat;
 public class ThreadLocalUserSessionTest {
 
   ThreadLocalUserSession threadLocalUserSession = new ThreadLocalUserSession();
-  @org.junit.Rule
+  @Rule
   public ExpectedException thrown = ExpectedException.none();
 
   @Before
   public void setUp() {
+    // for test isolation
+    threadLocalUserSession.remove();
+  }
+
+  @After
+  public void tearDown() throws Exception {
+    // clean up for next test
     threadLocalUserSession.remove();
   }