Vantaggio / svantaggio di avere tutte le variabili dichiarate in un test JUnit

8

Ho scritto alcuni test unitari per un nuovo codice al lavoro e l'ho inviato per una revisione del codice. Uno dei miei colleghi ha fatto un commento sul motivo per cui stavo inserendo le variabili che vengono utilizzate in un numero di questi test al di fuori dell'ambito del test.

Il codice che ho pubblicato era essenzialmente

import org.junit.Test;

public class FooUnitTests {

    @Test
    public void testConstructorWithValidName() {
        new Foo(VALID_NAME);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithNullName() {
        new Foo(null);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithZeroLengthName() {
        new Foo("");
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithLeadingAndTrailingWhitespaceInName() {
        final String name = " " + VALID_NAME + " ";
        final Foo foo = new Foo(name);

        assertThat(foo.getName(), is(equalTo(VALID_NAME)));
    }

    private static final String VALID_NAME = "name";
}

Le sue modifiche proposte erano essenzialmente

import org.junit.Test;

public class FooUnitTests {

    @Test
    public void testConstructorWithValidName() {
        final String name = "name";
        final Foo foo = new Foo(name);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithNullName() {
        final String name = null;
        final Foo foo = new Foo(name);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithZeroLengthName() {
        final String name = "";
        final Foo foo = new Foo(name);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithLeadingAndTrailingWhitespaceInName() {
        final String name = " name ";
        final Foo foo = new Foo(name);

        final String actual = foo.getName();
        final String expected = "name";

        assertThat(actual, is(equalTo(expected)));
    }
}

Dove tutto richiesto nell'ambito dello scopo del test, viene definito nell'ambito del test.

Alcuni dei vantaggi che sosteneva erano

  1. Ogni test è autonomo.
  2. Ogni test può essere eseguito separatamente o in aggregazione, con lo stesso risultato.
  3. Il revisore non deve scorrere fino a dove questi parametri sono dichiarati per cercare quale sia il valore.

Alcuni degli svantaggi del suo metodo che ho sostenuto erano

  1. Aumenta la duplicazione del codice
  2. Può aggiungere rumore ai revisori se ci sono diversi test simili con diversi valori definiti (es. test con doFoo(bar) si traduce in un valore, mentre la stessa chiamata ha un risultato diverso perché bar è definita diversamente in quel metodo).

Oltre alla convenzione, ci sono altri vantaggi / svantaggi dell'uso di entrambi i metodi rispetto all'altro?

    
posta Zymus 18.05.2015 - 09:52
fonte

3 risposte

9

Dovresti continuare a fare quello che stai facendo.

Ripetere se stessi è un'idea altrettanto brutta nel codice di prova come nel codice aziendale, e per lo stesso motivo. Il tuo collega è stato ingannato dall'idea che ogni test dovrebbe essere auto-contenuto . È vero, ma "autonomo" non significa che dovrebbe contenere tutto ciò di cui ha bisogno all'interno del suo corpo metodico. Significa solo che dovrebbe dare lo stesso risultato sia che venga eseguito in isolaton o come parte di una suite, indipendentemente dall'ordine dei test nella suite. In altre parole, il codice di test dovrebbe avere la stessa semantica indipendentemente da quale altro codice è stato eseguito prima di ; non deve avere tutto il codice richiesto in bundle testualmente .

Riutilizzare le costanti, il codice di configurazione e simili aumenta la qualità del codice di test e non compromette la sua autosufficienza, quindi è una buona cosa che dovresti continuare a fare.

    
risposta data 18.05.2015 - 10:01
fonte
2

Dipende. In generale, avere costanti ripetute dichiarate in un solo posto è una buona cosa.

Tuttavia, nel tuo esempio, non ha senso definire un membro VALID_NAME nel modo mostrato. Supposto nella seconda variante, un manutentore cambia il testo name nel primo test, il secondo test sarà probabilmente ancora valido, e viceversa. Ad esempio, si supponga di voler ulteriormente testare lettere maiuscole e minuscole, ma anche di tenere un caso di test solo con lettere minuscole, con il minor numero possibile di casi di test. Invece di aggiungere un nuovo test, puoi cambiare il primo test in

public void testConstructorWithValidName() {
    final String name = "Name";
    final Foo foo = new Foo(name);
}

e continua a conservare i test rimanenti.

In effetti, avere molti test a seconda di tale variabile e modificare il valore di VALID_NAME più tardi potrebbe interrompere involontariamente alcuni dei tuoi test in un secondo momento. E in una situazione del genere, puoi effettivamente migliorare l'autocontenimento dei tuoi test non introducendo una costante artificiale con test diversi a seconda di essa.

Tuttavia, ciò che @KilianFoth ha scritto sul non ripetere te stesso è corretto: segui il principio DRY nel codice di test nello stesso modo in cui lo fai nel codice aziendale. Ad esempio, introdurre costanti o variabili membro quando è essenziale per il codice di test che il loro valore è lo stesso in ogni luogo.

Il tuo esempio mostra come i test tendono a ripetere il codice di inizializzazione, che è un tipico punto di partenza per il refactoring. Quindi potresti considerare di rifattorizzare la ripetizione di

  new Foo(...);

in una funzione separata

 public Foo ConstructFoo(String name) 
 {
     return new Foo(name);
 }

dato che ti permetterà di cambiare più facilmente la firma del costruttore Foo in un secondo momento (perché ci sono meno posti dove viene effettivamente chiamato new Foo ).

    
risposta data 18.05.2015 - 13:06
fonte
1

In realtà ... non andare per entrambi , ma sceglierò il tuo sul tuo collega di lavoro per il tuo esempio semplificato.

Per prima cosa, tendo a privilegiare i valori di allineamento invece di fare assegnazioni una tantum. Ciò migliora la leggibilità del codice per me, dal momento che non devo interrompere il mio corso di pensiero in più istruzioni di assegnazione, allora leggere la / le riga / e del codice in cui sono utilizzati. Pertanto, preferirò il tuo approccio al tuo collega di lavoro, con il leggero nitpick che testConstructorWithLeadingAndTrailingWhitespaceInName() può probabilmente essere solo:

assertThat(new Foo(" " + VALID_NAME + " ").getName(), is(equalTo(VALID_NAME)));

Questo mi porta alla denominazione. Di solito, se il test sta facendo valere un Exception da gettare, il nome del test dovrebbe descrivere anche questo comportamento per chiarezza. Usando l'esempio sopra, quindi cosa succede se ho degli spazi bianchi extra nel nome quando costruisco il mio oggetto Foo ? E in che modo è legato al lancio di un IllegalArgumentException ?

Sulla base dei test null e "" , presumo qui che lo stesso Exception sia lanciato questa volta per chiamare getName() . Se è davvero così, forse sarà meglio impostare il nome del metodo di prova come callingGetNameWithWhitespacePaddingThrows() ( IllegalArgumentException - che ritengo possa far parte dell'output di JUnit, quindi opzionale). Se il riempimento di spazi vuoti nel nome durante l'istanziazione genererà anche lo stesso Exception , quindi salterò completamente l'asserzione, per chiarire che è nel comportamento del costruttore.

Tuttavia, penso che ci sia un modo migliore di fare le cose qui quando si ottengono più permutazioni per input validi e non validi, ovvero test parametrizzati . Quello che stai testando è esattamente questo: stai creando un gruppo di oggetti Foo con diversi argomenti del costruttore, e quindi asserendo che fallisce nell'istanza o chiamando getName() . Pertanto, perché non lasciare che JUnit esegua l'iterazione e l'impalcatura per te? Puoi, in parole povere, dire a JUnit, "questi sono i valori con cui voglio creare un Foo oggetto con", e poi fare in modo che il metodo di prova asserisca per IllegalArgumentException nelle posizioni giuste. Sfortunatamente, dato che il metodo di test parametrizzato di JUnit non funziona bene con i test per entrambi i casi di successo e eccezionali nello stesso test (per quanto ne so io), probabilmente dovrai saltare un po 'di la seguente struttura:

@RunWith(Parameterized.class)
public class FooUnitTests {

    private static final String VALID_NAME = "ABC";

    @Parameters
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][] {
                 { VALID_NAME, false }, 
                 { "", true }, 
                 { null, true }, 
                 { " " + VALID_NAME + " ", true }
           });
    }

    private String input;

    private boolean throwException;

    public FooUnitTests(String input, boolean throwException) {
        this.input = input;
        this.throwException = throwException;
    }

    @Test
    public void test() {
        try {
            Foo test = new Foo(input);
            // TODO any testing required for VALID_NAME?
            if (throwException) {
                assertEquals(VALID_NAME, test.getName());
            }
        } catch (IllegalArgumentException e) {
            if (!throwException) {
                throw new RuntimeException(e);
            }
        }
    }
}

Certo, sembra goffo per quello che è un test a quattro valori altrimenti semplice, e non è aiutato molto dall'implementazione alquanto restrittiva di JUnit. A questo proposito, trovo che l'approccio TestNG @DataProvider sia più utile, e vale sicuramente la pena un'occhiata più da vicino se stai considerando test parametrizzati.

Ecco come si potrebbe essere in grado di utilizzare TestNG per i test di unità (utilizzando Java 8 Stream per semplificare la costruzione di Iterator ). Alcuni dei vantaggi sono, ma non limitati a:

  1. I parametri del test diventano argomenti del metodo, non campi della classe.
  2. Puoi avere più @DataProvider s che forniscono diversi tipi di input.
    • È verosimilmente più chiaro e più semplice aggiungere nuovi input validi / non validi per il test.
  3. Sembra ancora un po 'esagerato per il tuo esempio semplificato, ma IMHO è più ordinato dell'approccio di JUnit.
public class FooUnitTests {

    private static final String VALID_NAME = "ABC";

    @DataProvider(name="successes")
    public static Iterator<Object[]> getSuccessCases() {
        return Stream.of(VALID_NAME).map(v -> new Object[]{ v }).iterator();
    }

    @DataProvider(name="exceptions")
    public static Iterator<Object[]> getExceptionCases() {
        return Stream.of("", null, " " + VALID_NAME + " ")
                    .map(v -> new Object[]{ v }).iterator();
    }

    @Test(dataProvider="successes")
    public void testSuccesses(String input) {
        new Foo(input);
        // TODO any further testing required?
    }

    @Test(dataProvider="exceptions", expectedExceptions=IllegalArgumentException.class)
    public void testExceptions(String input) {
        Foo test = new Foo(input);
        if (input.contains(VALID_NAME)) {
            // TestNG favors assert(actual, expected).
            assertEquals(test.getName(), VALID_NAME);
        }
    }
}
    
risposta data 19.05.2015 - 05:30
fonte

Leggi altre domande sui tag