Polish Tests and Error Messages

MockMvc matchers are best matched with the MockMvc execution API -
it's a little odd to try and use them inside of an AssertJ assertion
since they do their own asserting.

It's more readable to place "this." in front of member variables.

It's best to test just one class at a time in a unit test.

Issue: spring-projectsgh-4187

web/src/main/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandler.java

@@ -38,7 +38,7 @@ public final class HeaderWriterLogoutHandler implements LogoutHandler {
 	 * @throws {@link IllegalArgumentException} if headerWriter is null.
 	 */
 	public HeaderWriterLogoutHandler(HeaderWriter headerWriter) {
-		Assert.notNull(headerWriter, "headerWriter cannot be null.");
+		Assert.notNull(headerWriter, "headerWriter cannot be null");
 		this.headerWriter = headerWriter;
 	}
 

web/src/main/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriter.java

@@ -67,7 +67,7 @@ public final class ClearSiteDataHeaderWriter implements HeaderWriter {
 	 * @throws {@link IllegalArgumentException} if sources is null or empty.
 	 */
 	public ClearSiteDataHeaderWriter(String ...sources) {
-		Assert.notEmpty(sources, "Sources cannot be empty or null.");
+		Assert.notEmpty(sources, "sources cannot be empty or null");
 		this.requestMatcher = new SecureRequestMatcher();
 		this.headerValue = Stream.of(sources).map(this::quote).collect(Collectors.joining(", "));
 	}

web/src/test/java/org/springframework/security/web/authentication/logout/HeaderWriterLogoutHandlerTests.java

@@ -20,24 +20,23 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.security.core.Authentication;
-import org.springframework.security.web.header.writers.ClearSiteDataHeaderWriter;
+import org.springframework.security.web.header.HeaderWriter;
 
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
-import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header;
+import static org.mockito.Mockito.verify;
 
 /**
  *
  * @author Rafiullah Hamedy
+ * @author Josh Cummings
  *
  * @see {@link HeaderWriterLogoutHandler}
  */
 public class HeaderWriterLogoutHandlerTests {
-	private static final String HEADER_NAME = "Clear-Site-Data";
-
 	private MockHttpServletResponse response;
 	private MockHttpServletRequest request;
 
@@ -51,54 +50,19 @@ public void setup() {
 	}
 
 	@Test
-	public void createInstanceWhenHeaderWriterIsNullThenThrowsException() {
+	public void constructorWhenHeaderWriterIsNullThenThrowsException() {
 		this.thrown.expect(IllegalArgumentException.class);
-		this.thrown.expectMessage("headerWriter cannot be null.");
+		this.thrown.expectMessage("headerWriter cannot be null");
 
 		new HeaderWriterLogoutHandler(null);
 	}
 
 	@Test
-	public void createInstanceWhenSourceIsNullThenThrowsException() {
-		this.thrown.expect(IllegalArgumentException.class);
-		this.thrown.expectMessage("Sources cannot be empty or null.");
-
-		new HeaderWriterLogoutHandler(new ClearSiteDataHeaderWriter());
-	}
-
-	@Test
-	public void logoutWhenRequestIsNotSecureThenHeaderIsNotPresent() {
-		HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler(
-				new ClearSiteDataHeaderWriter("cache"));
-
-		handler.logout(request, response, mock(Authentication.class));
-
-		assertThat(header().doesNotExist(HEADER_NAME));
-	}
-
-	@Test
-	public void logoutWhenRequestIsSecureThenHeaderIsPresentMatchesWildCardSource() {
-		HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler(
-				new ClearSiteDataHeaderWriter("*"));
-
-		this.request.setSecure(true);
-
-		handler.logout(request, response, mock(Authentication.class));
-
-		assertThat(header().stringValues(HEADER_NAME, "\"*\""));
-	}
-
-	@Test
-	public void logoutWhenRequestIsSecureThenHeaderValueMatchesSource() {
-		HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler(
-				new ClearSiteDataHeaderWriter("cache", "cookies", "storage",
-						"executionContexts"));
-
-		this.request.setSecure(true);
-
-		handler.logout(request, response, mock(Authentication.class));
+	public void logoutWhenHasHeaderWriterThenInvoked() {
+		HeaderWriter headerWriter = mock(HeaderWriter.class);
+		HeaderWriterLogoutHandler handler = new HeaderWriterLogoutHandler(headerWriter);
+		handler.logout(this.request, this.response, mock(Authentication.class));
 
-		assertThat(header().stringValues(HEADER_NAME, "\"cache\", \"cookies\", \"storage\", "
-				+ "\"executionContexts\""));
+		verify(headerWriter).writeHeaders(this.request, this.response);
 	}
 }

web/src/test/java/org/springframework/security/web/header/writers/ClearSiteDataHeaderWriterTests.java

@@ -20,15 +20,16 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header;
 
 /**
  *
  * @author Rafiullah Hamedy
+ * @author Josh Cummings
  *
  * @see {@link ClearSiteDataHeaderWriter}
  */
@@ -43,53 +44,43 @@ public class ClearSiteDataHeaderWriterTests {
 
 	@Before
 	public void setup() {
-		request = new MockHttpServletRequest();
-		request.setSecure(true);
-		response = new MockHttpServletResponse();
+		this.request = new MockHttpServletRequest();
+		this.request.setSecure(true);
+		this.response = new MockHttpServletResponse();
 	}
 
 	@Test
 	public void createInstanceWhenMissingSourceThenThrowsException() {
 		this.thrown.expect(Exception.class);
-		this.thrown.expectMessage("Sources cannot be empty or null.");
+		this.thrown.expectMessage("sources cannot be empty or null");
 
 		new ClearSiteDataHeaderWriter();
 	}
 
-	@Test
-	public void createInstanceWhenEmptySourceThenThrowsException() {
-		this.thrown.expect(Exception.class);
-		this.thrown.expectMessage("Sources cannot be empty or null.");
-
-		new ClearSiteDataHeaderWriter(new String[] {});
-	}
-
 	@Test
 	public void writeHeaderWhenRequestNotSecureThenHeaderIsNotPresent() {
 		this.request.setSecure(false);
-
 		ClearSiteDataHeaderWriter headerWriter = new ClearSiteDataHeaderWriter("cache");
-		headerWriter.writeHeaders(request, response);
+		headerWriter.writeHeaders(this.request, this.response);
 
-		assertThat(header().doesNotExist(HEADER_NAME));
+		assertThat(this.response.getHeader(HEADER_NAME)).isNull();
 	}
 
 	@Test
 	public void writeHeaderWhenRequestIsSecureThenHeaderValueMatchesPassedSource() {
 		ClearSiteDataHeaderWriter headerWriter = new ClearSiteDataHeaderWriter("storage");
-		headerWriter.writeHeaders(request, response);
+		headerWriter.writeHeaders(this.request, this.response);
 
-		assertThat(header().stringValues(HEADER_NAME, "\"storage\""));
+		assertThat(this.response.getHeader(HEADER_NAME)).isEqualTo("\"storage\"");
 	}
 
 	@Test
 	public void writeHeaderWhenRequestIsSecureThenHeaderValueMatchesPassedSources() {
 		ClearSiteDataHeaderWriter headerWriter =
 				new ClearSiteDataHeaderWriter("cache", "cookies", "storage", "executionContexts");
+		headerWriter.writeHeaders(this.request, this.response);
 
-		headerWriter.writeHeaders(request, response);
-
-		assertThat(header().stringValues(HEADER_NAME, "\"cache\", \"cookies\", \"storage\","
-				+ " \"executionContexts\""));
+		assertThat(this.response.getHeader(HEADER_NAME))
+				.isEqualTo("\"cache\", \"cookies\", \"storage\", \"executionContexts\"");
 	}
 }