1
\$\begingroup\$

I have a quite large unit test case for one class that currently does not exist, I am going to write it after finishing the test case.

I am wondering if my unit test doesn't lack something important, or alternatively, if it is not too complex. I have to describe what the tested class is supposed to do, although some details are left out, and I can answer questions about them if needed.

This class is a key matcher, part of a library implementing things like json web signing/json web key.

The key matcher will take input from some other classes like JWS processors, that will process json objects. The json representation of, for example, a signed object, can contain a key used to verify the signatures on the object. This key can be given as a json web key or certificate, or can be given by key id. A key set can also be provided, that would mean the key identified by the key id is to be looked up in that set. The protocol does not specify the policy of determining which key to use for verification, so in theory all those fields can be present at once.

The key matcher will match keys, trying in some predetermined order, like explicit key has priority over a certificate, and it has priority over matching in key set, last is an external source of keys or certificates that is application specific. However, usually when the matcher looks for keys in different sources, and a higher priority source is found, a lower priority source is usually not tried even if keys from the higher priority source do not match. It stems from the fact that if an application sends a signed or encrypted object containing multiple incompatible keys, it is an application error and such cases should not be handled, because they make no sense.

This test case tests matching in case of each possible source of key material, and most but not all cases of match failure. However, it does not test for cases where matcher would throw NullPointerException or IllegalArgumentException or possibly IllegalStateException, for example if a key type is not specified. I am not sure if I should test for good reaction to bugs in the user of the matcher class. The last test will test if the order of matching is correct.

I would like to know if my unit test too complex, or if it misses something important.

/**
* Copyright (c) 2016-2017, acme-client developers
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:
*
* 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
*
* 2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package io.github.webczat.acmeClient.jws.keyMatching;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.cert.X509Certificate;
import java.util.*;

import org.junit.Test;

import io.github.webczat.acmeClient.jws.KeyType;
import io.github.webczat.acmeClient.jws.NoMatchingKeyException;
import io.github.webczat.acmeClient.jws.WebKey;
import io.github.webczat.acmeClient.jws.WebPublicKey;
import io.github.webczat.acmeClient.testUtil.CertificateTestUtils;

/**
 * This class tests key matcher.
 * 
 * @author webczat
 */
@SuppressWarnings({ "javadoc" })
public class KeyMatcherTest {
    /**
     * Test for matching an explicitly given key.
     */
    @Test
    public void testExplicitKeyMatchWithAlgorithm() {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.RSA);
        when(key.getAlgorithm()).thenReturn("test");
        assertEquals(new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setWebKey(key).match(), key);
    }

    /*
     * Test for explicitly given key without match without algorithm on the key.
     */
    @Test
    public void testExplicitKeyMatchWithoutAlgorithm() {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.RSA);
        assertEquals(new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setWebKey(key).match(), key);
    }

    /**
     * Test explicit key match with bad algorithm.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testExplicitKeyMatchWithBadAlgorithm() throws NoMatchingKeyException {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.RSA);
        when(key.getAlgorithm()).thenReturn("test2");
        new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test2").setWebKey(key).match();
    }

    /**
     * Test for explicit key with bad key type.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testExplicitKeyMatchWithBadType() throws NoMatchingKeyException {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.EC);
        new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").match();
    }

    /**
     * Tests for explicit key match with a key validator passing.
     */
    @Test
    public void testExplicitKeyMatchWithPassingValidator() {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.RSA);
        assertEquals(
                new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setKeyValidator((k) -> true).setWebKey(
                        key).match(),
                key);
    }

    /**
     * Test for explicit key match with failing validator.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testExplicitKeyMatchWithFailingValidator() throws NoMatchingKeyException {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.RSA);
        new KeyMatcher().setWebKey(key).setKeyValidator((k) -> false).setAlgorithm("test").setKeyType(
                KeyType.RSA).match();
    }

    /**
     * Test for matching a key from the given set of keys, with given key
     * identifier.
     * 
     */
    @Test
    public void testSetKeyMatchWithKeyId() {
        WebPublicKey key1 = mock(WebPublicKey.class), key2 = mock(WebPublicKey.class), key3 = mock(WebPublicKey.class);
        when(key1.getKeyType()).thenReturn(KeyType.RSA);
        when(key1.getKeyId()).thenReturn("test");
        when(key2.getKeyType()).thenReturn(KeyType.EC);
        when(key2.getKeyId()).thenReturn("test");
        when(key3.getKeyType()).thenReturn(KeyType.RSA);
        LinkedHashSet<WebKey> keySet = new LinkedHashSet<WebKey>(
                Arrays.asList(new WebKey[] { key3, key2, key1, key1 }));
        assertEquals(new KeyMatcher().setKeyId("test").setKeyType(KeyType.RSA).setAlgorithm("test").setWebKeySet(
                keySet).match(), key1);
    }

    /**
     * Test for no matching keys for key id when matching by key set.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testSetKeyMatchWithKeyIdAndNoCandidates() throws NoMatchingKeyException {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.RSA);
        LinkedHashSet<WebKey> keySet = new LinkedHashSet<WebKey>(Arrays.asList(new WebKey[] { key }));
        new KeyMatcher().setAlgorithm("test").setKeyId("test").setKeyType(KeyType.RSA).setWebKeySet(keySet).match();
    }

    /**
     * Tests key matching using a key set, with no key id given.
     */
    @Test
    public void testSetKeyMatchWithoutKeyId() {
        WebPublicKey key1 = mock(WebPublicKey.class), key2 = mock(WebPublicKey.class);
        when(key1.getKeyType()).thenReturn(KeyType.RSA);
        when(key2.getKeyType()).thenReturn(KeyType.EC);
        LinkedHashSet<WebKey> keySet = new LinkedHashSet<>(Arrays.asList(new WebKey[] { key2, key1 }));
        assertEquals(new KeyMatcher().setAlgorithm("test").setKeyType(KeyType.RSA).setWebKeySet(keySet).match(), key1);
    }

    /**
     * Test for matching keys from set with no key id and no candidates.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testSetKeyMatchWithoutKeyIdAndCandidates() throws NoMatchingKeyException {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.EC);
        HashSet<WebKey> keySet = new HashSet<>(Arrays.asList(new WebKey[] { key }));
        new KeyMatcher().setAlgorithm("test").setKeyType(KeyType.RSA).setWebKeySet(keySet).match();
    }

    /**
     * Test for matching key from external source.
     */
    @Test
    public void testExternalKeyMatch() {
        WebPublicKey key1 = mock(WebPublicKey.class), key2 = mock(WebPublicKey.class);
        when(key1.getKeyType()).thenReturn(KeyType.RSA);
        when(key2.getKeyType()).thenReturn(KeyType.EC);
        KeyProvider kp = mock(KeyProvider.class);
        when(kp.lookupKey("test")).thenReturn(Arrays.asList(new WebKey[] { key2, key1 }));
        assertEquals(new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setKeyId("test").setKeyProvider(
                kp).match(), key1);
    }

    /**
     * Test matching keys from external source when no keys match.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testExternalKeyMatchWithNoCandidates() throws NoMatchingKeyException {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.EC);
        KeyProvider kp = mock(KeyProvider.class);
        when(kp.lookupKey("test")).thenReturn(Arrays.asList(new WebKey[] { key }));
        new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setKeyId("test").setKeyProvider(kp).match();
    }

    /**
     * Test for external key matching when no key id specified, it should not
     * work at all.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testExternalKeyMatchWithNoKeyId() throws NoMatchingKeyException {
        WebPublicKey key = mock(WebPublicKey.class);
        when(key.getKeyType()).thenReturn(KeyType.RSA);
        KeyProvider kp = mock(KeyProvider.class);
        when(kp.lookupKey("test")).thenReturn(Arrays.asList(new WebKey[] { key }));
        new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setKeyProvider(kp).match();
    }

    /**
     * Test for certificate matching when cert chain is explicitly given.
     */
    @Test
    public void testExplicitCertificateMatch() {
        List<X509Certificate> certs = CertificateTestUtils.newChain(3, null).getCertificateChain();
        assertEquals(new KeyMatcher().setAlgorithm("test").setKeyType(KeyType.RSA).setCertificateChain(certs).match(),
                certs);
    }

    /**
     * Test explicit certificate match that fails.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testExplicitCertificateMatchFailure() throws NoMatchingKeyException {
        List<X509Certificate> certs = CertificateTestUtils.newChain(3, null).getCertificateChain();
        new KeyMatcher().setKeyType(KeyType.EC).setAlgorithm("test").setCertificateChain(certs).match();
    }

    /**
     * Test SHA256 fingerprint matching.
     */
    @Test
    public void testFingerprintCertificateMatchWithSha256() throws NoSuchAlgorithmException {
        List<X509Certificate> certs = CertificateTestUtils.newChain(3, null).getCertificateChain();
        byte[] fingerprint = MessageDigest.getInstance("SHA2-256").digest(certs.get(0).getEncoded());
        CertificateProvider cp = mock(CertificateProvider.class);
        when(cp.lookupCertificateBySha256Fingerprint(fingerprint)).thenReturn(certs);
        assertEquals(new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setSha256Fingerprint(
                fingerprint).setCertificateProvider(cp).match(), certs);
    }

    /**
     * Test certificate matching using SHA1 fingerprint.
     */
    @Test
    public void testFingerprintCertificateMatchWithSha1() throws NoSuchAlgorithmException {
        List<X509Certificate> certs = CertificateTestUtils.newChain(3, null).getCertificateChain();
        byte[] fingerprint = MessageDigest.getInstance("SHA1").digest(certs.get(0).getEncoded());
        CertificateProvider cp = mock(CertificateProvider.class);
        when(cp.lookupCertificateBySha1Fingerprint(fingerprint)).thenReturn(certs);
        assertEquals(new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setCertificateProvider(
                cp).setSha1Fingerprint(fingerprint).match(), certs);
    }

    /**
     * Test for matching by sha256 fingerprint when key type is invalid.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testFingerprintCertificateMatchWithSha256AndBadKeyType()
            throws NoMatchingKeyException, NoSuchAlgorithmException {
        List<X509Certificate> certs = CertificateTestUtils.newChain(3, null).getCertificateChain();
        byte[] fingerprint = MessageDigest.getInstance("SHA2-256").digest(certs.get(0).getEncoded());
        CertificateProvider cp = mock(CertificateProvider.class);
        when(cp.lookupCertificateBySha256Fingerprint(fingerprint)).thenReturn(certs);
        new KeyMatcher().setKeyType(KeyType.EC).setCertificateProvider(cp).setAlgorithm("test").setSha256Fingerprint(
                fingerprint).match();
    }

    /**
     * Test for sha1 fingerprint matching when wrong key type is given.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testFingerprintCertificateMatchWithSha1AndBadKeyType()
            throws NoMatchingKeyException, NoSuchAlgorithmException {
        List<X509Certificate> certs = CertificateTestUtils.newChain(3, null).getCertificateChain();
        byte[] fingerprint = MessageDigest.getInstance("SHA1").digest(certs.get(0).getEncoded());
        CertificateProvider cp = mock(CertificateProvider.class);
        when(cp.lookupCertificateBySha1Fingerprint(fingerprint)).thenReturn(certs);
        new KeyMatcher().setCertificateProvider(cp).setKeyType(KeyType.EC).setAlgorithm("test").setSha1Fingerprint(
                fingerprint).match();
    }

    /**
     * Test for fingerprint matching with no fingerprints set.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testFingerprintCertificateMatchWithNoFingerprint() {
        new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").setCertificateProvider(
                mock(CertificateProvider.class)).match();
    }

    /**
     * Test for matching when no parameters are set.
     */
    @Test(expected = NoMatchingKeyException.class)
    public void testKeyMatchWithNoData() throws NoMatchingKeyException {
        new KeyMatcher().setKeyType(KeyType.RSA).setAlgorithm("test").match();
    }

    /**
     * Test for matching order.
     */
    @Test
    public void testMatchingOrder() {
        WebPublicKey key1 = mock(WebPublicKey.class);
        when(key1.getKeyType()).thenReturn(KeyType.RSA);
        KeyMatcher km = new KeyMatcher();
        km.setKeyType(KeyType.RSA).setAlgorithm("TEST").setWebKey(key1);
        List<X509Certificate> certs1 = CertificateTestUtils.newChain(1, null).getCertificateChain();
        km.setCertificateChain(certs1);
        WebPublicKey key2 = mock(WebPublicKey.class);
        when(key2.getKeyType()).thenReturn(KeyType.RSA);
        when(key2.getKeyId()).thenReturn("test");
        Set<WebKey> keySet = new HashSet<>();
        keySet.add(key2);
        km.setWebKeySet(keySet);
        km.setKeyId("test");
        List<X509Certificate> certs2 = CertificateTestUtils.newChain(1, null).getCertificateChain();
        byte[] fingerprint1 = MessageDigest.getInstance("SHA2-256").digest(certs2.get(0).getEncoded());
        km.setSha256Fingerprint(fingerprint1);
        List<X509Certificate> certs3 = CertificateTestUtils.newChain(1, null).getCertificateChain();
        byte[] fingerprint2 = MessageDigest.getInstance("SHA1").digest(certs3.get(0).getEncoded());
        km.setSha1Fingerprint(fingerprint2);
        CertificateProvider cp = mock(CertificateProvider.class);
        when(cp.lookupCertificateBySha256Fingerprint(fingerprint1)).thenReturn(certs2);
        when(cp.lookupCertificateBySha1Fingerprint(fingerprint2)).thenReturn(certs3);
        km.setCertificateProvider(cp);
        WebPublicKey key3 = mock(WebPublicKey.class);
        when(key3.getKeyType()).thenReturn(KeyType.RSA);
        KeyProvider kp = mock(KeyProvider.class);
        when(kp.lookupKey("test")).thenReturn(Arrays.asList(new WebKey[] {key3}));
        km.setKeyProvider(kp);
        assertEquals(km.match(), key1);
        km.setWebKey(null);
        assertEquals(km.match(), certs1);
        km.setCertificateChain(null);
        assertEquals(km.match(), key2);
        km.setWebKeySet(null);
        assertEquals(km.match(), certs2);
        km.setSha256Fingerprint(null);
        assertEquals(km.match(), certs3);
        km.setSha1Fingerprint(null);
        assertEquals(km.match(), key3);
    }
}
\$\endgroup\$
2
\$\begingroup\$

I really like the small test methods. But without seeing the actual implementation, it is very hard to tell, if a test case makes sense or does what it should do.

Small improvements:

  • Split your test methods into three blocks, when-given-then and use a empty line between those. It can help a lot, not always, but I recommend to do it. It's like using your indicator: Even though noone is around (= quite an easy test case), it's a good habit to use it always, so you will use it, when it's actually needed.
  • You can make WebPublicKey an instance variable and use the @Mock annotation. In the setup (@Before), you can use MockitoAnnotations.initMocks(this);. So you can save the first line of every test.
  • I have a hard time to understand, what match() does, or what it should do (= the intention is not clear). Why must match() equal key? When I read matches(), I expect to have a boolean returned. Shouldn't it be something like findMatchingKey() or something?
  • The test-prefix of your test cases aren't needed, it's used "back in the day", before annotations were a thing in java/junit. Instead of testExplicitKeyMatchWithAlgorithm, you can write explicitKeyMatchesWithAlgorithm
  • The JavaDoc for the methods are first of all, most of the times it is JavaDoc, but not always. 2nd: I'm 99% sure, noone will ever read those java docs (you guys do even generate those for test cases?). 3rd: "Test for matching an explicitly given key." vs "testExplicitKeyMatchWithAlgorithm". So, you have a comment, a method name, and the actual code. Rhetorical question: which one is true? The java doc does not talk about algorithm, the method name does.
  • The "test" algorithm: I usually declare those explicitly as variable in the test case, so the reader sees where it's used (WebPublicKey and KeyMatcher)
  • There's a lof of repetition of the WebPublicKey instantiation (RSA KeyType and test algorithm), you might want to add a static helper method for that, something like rsaWithTestAlgorithmKeyMatcher(), or even add a constant. For the other creations, I'd provide a method like keyMatcher(keyType: KeyType, algorithm: String): KeyMatcher.
  • testMatchingOrder: Now, that's quite the confusing test method. You wrote, that the implementation does not exist. The actual test driven approach would be "write failing test case, implement, refactor". Now important is, you write one failing test case. And you only implement what is needed for the test case to run ("Know when to stop."), so you do not implement too much. And an important thing, too, is: "What do I need to change, to make my test fail". I mention all that especially because of the last test case. Do you actually have to use four different Keys, wouldn't be two enough to ensure the correct return value? If not: Then something's different, you have to consider writing two different test cases.
  • testMatchingOrder 2: Also to point out the other points I've mentioned, especially the helper-methods and then give-when-then block: Those applied should make this method a lot easier to understand. Beside that: There's nothing wrong to write keyMatcher instead of km, certificateProvider instead of cp and so on. It certainly would have helped me.
  • testMatchingOrder 3: After reading that, I still do not understand the expected behavior of the KeyMatcher, especiall the latest part. You call match, and expect key1. Why? Then you set the webKey to null (why?), and then you expect certs1 (why?). The setup of the test doesn't really help to understand it either.

Hope that helps,...

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ First, what about tests for things like methods given wrong parameters or in this case key matcher being in a wrong state because algorithm and key type are not set? they are required. not sure if I have to actually test those or not. About your points: I do not quite understand the first one about splitting test methods. About the usage of mock annotations, I use them like that in every test requiring mocks except this one. Mainly for the reason that here, depending on the test, there are variations of number of inputs and their specific parameters. although that may still be a good thing. \$\endgroup\$ – Michał Zegan May 3 '17 at 1:05
  • \$\begingroup\$ continuing: As for match method, it will match/select the most appropriate key, and return it. it may be a WebKey or a certificate so it will probably need to return Object or a special wrapper. Method names are just a convention because I like it, and I use annotations when I can. Javadocs are just short informative especially that maven likes to generate test javadocs, but notice the warning suppression, I do not write comprehensive javadocs there, nor use javadoc tags. But maybe they need to be improved. My experience says many javadocs of libraries themselves are worse than that. \$\endgroup\$ – Michał Zegan May 3 '17 at 1:08
  • \$\begingroup\$ Probably the last in series. :) most test methods test one single kind of key matching in actual separation. The last one only tests the priority of key sources. like, if given both a WebKey (jwk object), a certificate chain and an external source, which will it try in what order? but, in most cases, when you set a web key, then certificates are not checked even if it does not match. That is why I do multiple asserts mixed with setting next key source in the list to null, but leaving all lower priority sources active. \$\endgroup\$ – Michał Zegan May 3 '17 at 1:15
  • \$\begingroup\$ ahh and as for write one failing test case/implement/refactor, this test class is quite big, and it could be a good practice to just delete it and write again with the above in mind, for practicing purposes, especially that I still have no implementation. \$\endgroup\$ – Michał Zegan May 3 '17 at 1:19
  • \$\begingroup\$ "First": Well, that's up to you, to define the expected behavior. "Test methods": Don't split the test methods itself, but format it into these three blocks. Declare the 'scenario' (given) first, execute the actual scenario (when) and assert it (then). Try it out and compare it, it's prettier and easiert to read. "Mocks": Well, you should at least mock it empty, it will be reset for each test method anyway. \$\endgroup\$ – slowy May 3 '17 at 8:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.