From 92d80b2f514c0208b0f759442cc80195f86d7545 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Thu, 28 May 2015 16:51:45 +0200 Subject: [PATCH] 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 --- .../sonar/server/user/DoPrivilegedTest.java | 76 +++++++++++++------ .../server/user/RubyUserSessionTest.java | 10 +++ .../user/ThreadLocalUserSessionTest.java | 11 ++- 3 files changed, 72 insertions(+), 25 deletions(-) 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 9ca8fb4c95a..1c15f020e07 100644 --- 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 d75a7c733b1..ac33f8d4078 100644 --- 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 1b69404d8c1..0347c0a5c4b 100644 --- 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(); } -- 2.39.5