From a278b95161276c03539ebf461d1eb4aff9cd55a1 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 22 Oct 2015 11:02:51 -0700 Subject: [PATCH] Distinguish between assertProblems and assertNoProblems 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 --- .../gpg/GerritPublicKeyCheckerTest.java | 10 +-- .../gerrit/gpg/PublicKeyCheckerTest.java | 61 +++++++++++++------ .../gpg/PushCertificateCheckerTest.java | 23 +++++-- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java index db0bb88ef7..4df9d37633 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java +++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java @@ -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 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(); } diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java index 576ffbd488..742bf1aa8b 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java +++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java @@ -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 list(String first, String[] rest) { + List all = new ArrayList<>(); + all.add(first); + all.addAll(Arrays.asList(rest)); + return all; + } } diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java index 977895859d..ee07d5580d 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java +++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java @@ -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 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()); } }