opendev/gerrit
Code Issues Proposed changes

Distinguish between assertProblems and assertNoProblems

Browse Source 
assertProblems(foo) with an implicit empty list can be confusing to
read. You can tell I figured this out, because
GerritPublicKeyCheckerTest, which postdates the other tests, already
used this pattern. Use it consistently everywhere.

Use (String, String...) in method signatures to make invalid use of
assertProblems with no expected problems a compile-time error.

Change-Id: I5621c586ac16dff2a34d30efd1cca8949907316d
This commit is contained in:
Dave Borowitz
2015-10-22 11:02:51 -07:00
parent 6ad5d8b90d
commit a278b95161
3 changed files with 67 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
View File

@@ -14,7 +14,6 @@
package com.google.gerrit.gpg;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.gpg.GerritPublicKeyChecker.toExtIdKey;
import static com.google.gerrit.gpg.PublicKeyStore.keyToString;
@@ -67,6 +66,7 @@ import org.junit.Before;
import org.junit.Test;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
@@ -433,11 +433,13 @@ public class GerritPublicKeyCheckerTest {
}
private void assertProblems(CheckResult result, Status expectedStatus,
String... expectedProblems) throws Exception {
checkArgument(expectedProblems.length > 0);
String first, String... rest) throws Exception {
List<String> expectedProblems = new ArrayList<>();
expectedProblems.add(first);
expectedProblems.addAll(Arrays.asList(rest));
assertThat(result.getStatus()).isEqualTo(expectedStatus);
assertThat(result.getProblems())
.containsExactly((Object[]) expectedProblems)
.containsExactlyElementsIn(expectedProblems)
.inOrder();
}

61
gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java
View File

@@ -54,10 +54,13 @@ import org.junit.Test;
import org.junit.rules.ExpectedException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
public class PublicKeyCheckerTest {
@@ -87,7 +90,7 @@ public class PublicKeyCheckerTest {
@Test
public void validKey() throws Exception {
assertProblems(validKeyWithoutExpiration());
assertNoProblems(validKeyWithoutExpiration());
}
@Test
@@ -96,10 +99,10 @@ public class PublicKeyCheckerTest {
PublicKeyChecker checker = new PublicKeyChecker()
.setStore(store);
assertProblems(checker, k);
assertNoProblems(checker, k);
checker.setEffectiveTime(parseDate("2015-07-10 12:00:00 -0400"));
assertProblems(checker, k);
assertNoProblems(checker, k);
checker.setEffectiveTime(parseDate("2075-07-10 12:00:00 -0400"));
assertProblems(checker, k, "Key is expired");
@@ -138,10 +141,10 @@ public class PublicKeyCheckerTest {
save();
PublicKeyChecker checker = newChecker(2, kb, kd);
assertProblems(checker, ka);
assertNoProblems(checker, ka);
assertProblems(checker, kb, "Key is expired");
assertProblems(checker, kc);
assertProblems(checker, kd);
assertNoProblems(checker, kc);
assertNoProblems(checker, kd);
assertProblems(checker, ke, "Key is expired", "No path to a trusted key");
}
@@ -192,7 +195,7 @@ public class PublicKeyCheckerTest {
// J trusts I to a depth of 1, so I itself is valid, but I's certification
// of K is not valid.
assertProblems(checker, ki);
assertNoProblems(checker, ki);
assertProblems(checker, kh,
"No path to a trusted key", notTrusted(ki));
}
@@ -212,7 +215,7 @@ public class PublicKeyCheckerTest {
save();
// Key no longer specified as revoker.
assertProblems(kr.getPublicKey());
assertNoProblems(kr.getPublicKey());
}
@Test
@@ -266,7 +269,7 @@ public class PublicKeyCheckerTest {
PublicKeyChecker checker = new PublicKeyChecker()
.setStore(store)
.setEffectiveTime(parseDate("2010-01-01 12:00:00 -0400"));
assertProblems(checker, k);
assertNoProblems(checker, k);
}
@Test
@@ -277,7 +280,7 @@ public class PublicKeyCheckerTest {
save();
PublicKeyChecker checker = new PublicKeyChecker().setStore(store);
assertProblems(checker, k);
assertNoProblems(checker, k);
}
@Test
@@ -293,7 +296,7 @@ public class PublicKeyCheckerTest {
// Set time between key creation and revocation.
checker.setEffectiveTime(parseDate("2005-08-01 13:00:00 -0400"));
assertProblems(checker, k);
assertNoProblems(checker, k);
}
private PGPPublicKeyRing removeRevokers(PGPPublicKeyRing kr) {
@@ -342,21 +345,38 @@ public class PublicKeyCheckerTest {
}
private void assertProblems(PublicKeyChecker checker, TestKey k,
String... expected) {
String first, String... rest) {
CheckResult result = checker.setStore(store)
.check(k.getPublicKey());
assertEquals(Arrays.asList(expected), result.getProblems());
assertEquals(list(first, rest), result.getProblems());
}
private void assertProblems(TestKey tk, String... expected) throws Exception {
assertProblems(tk.getPublicKey(), expected);
private void assertNoProblems(PublicKeyChecker checker, TestKey k) {
CheckResult result = checker.setStore(store)
.check(k.getPublicKey());
assertEquals(Collections.emptyList(), result.getProblems());
}
private void assertProblems(PGPPublicKey k, String... expected) throws Exception {
private void assertProblems(TestKey tk, String first, String... rest) {
assertProblems(tk.getPublicKey(), first, rest);
}
private void assertNoProblems(TestKey tk) {
assertNoProblems(tk.getPublicKey());
}
private void assertProblems(PGPPublicKey k, String first, String... rest) {
CheckResult result = new PublicKeyChecker()
.setStore(store)
.check(k);
assertEquals(Arrays.asList(expected), result.getProblems());
assertEquals(list(first, rest), result.getProblems());
}
private void assertNoProblems(PGPPublicKey k) {
CheckResult result = new PublicKeyChecker()
.setStore(store)
.check(k);
assertEquals(Collections.emptyList(), result.getProblems());
}
private static String notTrusted(TestKey k) {
@@ -367,4 +387,11 @@ public class PublicKeyCheckerTest {
private static Date parseDate(String str) throws Exception {
return new SimpleDateFormat("YYYY-MM-dd HH:mm:ss Z").parse(str);
}
private static List<String> list(String first, String[] rest) {
List<String> all = new ArrayList<>();
all.add(first);
all.addAll(Arrays.asList(rest));
return all;
}
}

23
gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java
View File

@@ -49,8 +49,11 @@ import java.io.ByteArrayOutputStream;
import java.io.InputStreamReader;
import java.io.Reader;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.List;
public class PushCertificateCheckerTest {
private InMemoryRepository repo;
@@ -98,7 +101,7 @@ public class PushCertificateCheckerTest {
public void validCert() throws Exception {
PushCertificate cert =
newSignedCert(validNonce(), validKeyWithoutExpiration());
assertProblems(cert);
assertNoProblems(cert);
}
@Test
@@ -113,7 +116,7 @@ public class PushCertificateCheckerTest {
checker = newChecker(false);
PushCertificate cert =
newSignedCert("invalid-nonce", validKeyWithoutExpiration());
assertProblems(cert);
assertNoProblems(cert);
}
@Test
@@ -139,7 +142,7 @@ public class PushCertificateCheckerTest {
Date now = new SimpleDateFormat("YYYY-MM-dd HH:mm:ss Z")
.parse("2005-07-10 12:00:00 -0400");
PushCertificate cert = newSignedCert(validNonce(), key3, now);
assertProblems(cert);
assertNoProblems(cert);
}
private String validNonce() {
@@ -193,9 +196,17 @@ public class PushCertificateCheckerTest {
return parser.parse(reader);
}
private void assertProblems(PushCertificate cert, String... expected)
throws Exception {
private void assertProblems(PushCertificate cert, String first,
String... rest) throws Exception {
List<String> expected = new ArrayList<>();
expected.add(first);
expected.addAll(Arrays.asList(rest));
CheckResult result = checker.check(cert).getCheckResult();
assertEquals(Arrays.asList(expected), result.getProblems());
assertEquals(expected, result.getProblems());
}
private void assertNoProblems(PushCertificate cert) {
CheckResult result = checker.check(cert).getCheckResult();
assertEquals(Collections.emptyList(), result.getProblems());
}
}