Endre søknadsbehandlingserviceiverksett-tester #664

simejo · 2022-01-27T15:00:01Z

Litt usikker på hvor revolusjonerende dette egentlig ble, men fiksa nå hvert fall dette:

Happy-casene har verify på alle metodene, og også argumenter der det er nødvendig (litt sånn som før)
Ingen av testene som tester en feil har verifys. Hvorfor? Fordi hvis man testen kaller på en metode som ikke er mocket, så vil mocken feile. OBS: Dette stemmer ikke nødvendigvis for Unit-metoder. Kanskje vi burde ha som regel at vi unngår Unit-metoder og heller alltid returnerer noe?
- Det skal selvsagt være lov å legge til noen never-verifys, men her er det egentlig bare publiserUtbetaling som er kritisk (siden lagre til databasen hos er i transaction). Den er ikke Unit og vil derfor krasje mocken om den blir kalt på. Ergo er det ikke behov for verify
Fjernet "egenproduserte" dataobjekter, og erstattet med de i TestData.

Fila ble nesten halvparten så stor, og personlig synes jeg testene er mye enklere å lese. Og ikke minst enklere å jobbe med.
Innspill?

hestad · 2022-01-27T18:17:23Z

Jeg er ikke veldig for å endre returtyper i implementasjonen. For at det stub-argumentet skal holde vann, kan vi gjøre noe som dette: Answer<Any> { fail("Unstubbed method: ${it.method}") }

/**
 * Non-stubbed methods fails
 */
inline fun <reified T : Any> mock(
    extraInterfaces: Array<out KClass<out Any>>? = null,
    name: String? = null,
    spiedInstance: Any? = null,
    defaultAnswer: Answer<Any>? = Answer<Any> { fail("Unstubbed method: ${it.method}") },
    serializable: Boolean = false,
    serializableMode: SerializableMode? = null,
    verboseLogging: Boolean = false,
    invocationListeners: Array<InvocationListener>? = null,
    stubOnly: Boolean = false,
    @Incubating useConstructor: UseConstructor? = null,
    @Incubating outerInstance: Any? = null,
    @Incubating lenient: Boolean = false,
): T {
    return org.mockito.kotlin.mock(
        extraInterfaces = extraInterfaces,
        name = name,
        spiedInstance = spiedInstance,
        defaultAnswer = defaultAnswer,
        serializable = serializable,
        serializableMode = serializableMode,
        verboseLogging = verboseLogging,
        invocationListeners = invocationListeners,
        stubOnly = stubOnly,
        useConstructor = useConstructor,
        outerInstance = outerInstance,
        lenient = lenient

    )
}

hestad · 2022-01-27T18:20:02Z

Jeg er også nysgjerrig på Mockito sin strict mode.

Kan legge til denne i build.gradle.kts: testImplementation("org.mockito:mockito-junit-jupiter:4.3.1")

Og deretter denne over testklassene: @ExtendWith(MockitoExtension::class)

hestad · 2022-01-27T19:02:24Z

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

-            grunnlagsdata = Grunnlagsdata.IkkeVurdert,
-            vilkårsvurderinger = Vilkårsvurderinger.Søknadsbehandling.IkkeVurdert,
-        )
+        val expected = søknadsbehandlingIverksattInnvilget().second.copy(id = behandling.id)


Hmm 🤔 Dette kan jo potensielt være litt skjørt. Men jeg ser kodelinjebesparelsen :]

Gitt at testdataene henger sammen så skal det være greit. Så må jo ikke testdataene endres da selvsagt😛 Grunnen til den copy-id tingen er at id-blir generert hver gang, og vil derfor bli diff

Trenger vi egentlig å matche hele objektet, eller kunne det holde at det er av riktig type f.eks.?

Det er jo servicen som blant annet er ansvarlig for å sette riktig attestant. Men dersom det byttes til å bruke en copy, vil jo ikke denne testen avdekke det. Så jeg er for at vi sjekker på hele objektet, spesielt når det er så enkelt. Da får vi og testet sammenhengen mellom testdataene 🤷

Vi kan ha egne tester på at spesifikke ting blir satt på objektet, sånn som f.eks. riktig attestant. Det er kanskje en detalj, men jeg vil nok heller at vi har spesifikke tester enn at det ser ut som vi matcher på hele objektet litt tilfeldig.

Jeg likte denne jeg :) Den løser i hvert fall at a) en test feiler dersom uønskede felter endres og b) en test feiler dersom vi ikke legger til attestanten som kom fra routen. Her burde vi nok heller brukt parametre inn mot funksjonen (id+attestant).

Men hvor mange tester vi har for å sikre dette, spiller ikke så mye rolle for meg.

Ja, tenker det er en filleting mot et par andre tilfeller vi har i testene, men helt generelt ville jeg heller at vi hadde hatt det sånn at spesifikk test X, Y og Z feiler dersom hhv. X, Y og Z går galt, enn at generell test A feiler hvis enten X, Y eller Z går galt.

Men tenker det er OK sånn, jeg hadde ikke tatt opp denne tingen hvis vi ikke hadde startet diskusjonen rundt "new opp hele objektet" vs. copy.

Dersom vi deler opp testene på denne måten er vi da priset at testene X,Y og Z har samme setup (typisk samme private funksjon). Noe som også gir mening i forbindelse med refaktoreringer.

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

hestad · 2022-01-27T19:17:08Z

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

-                vedtak = it as VedtakSomKanRevurderes.EndringIYtelse.InnvilgetSøknadsbehandling
-                it.behandling shouldBe expected
-            },
+        verify(serviceAndMocks.utbetalingService).lagreUtbetaling(any(), anyOrNull())


En idé jeg hadde var at vi nå har mulighet til å sjekke at session-objektet på disse TransactionContext er et samme (da verifiserer vi i hvert fall at ting er under samme transaksjonen)

Det er sant, men noen idé om hvordan vi får testet det lett uten å måtte lage en mutable variabel?

Må ha en mutable variabel på et eller annet vis da ja. Kan jo prøve en spy() f.eks.

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

christianmosveen

Liker veldig godt fjerningen av verify der det ikke er nødvendig.

christianmosveen · 2022-01-28T09:08:25Z

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

-            grunnlagsdata = Grunnlagsdata.IkkeVurdert,
-            vilkårsvurderinger = Vilkårsvurderinger.Søknadsbehandling.IkkeVurdert,
-        )
+        val expected = søknadsbehandlingIverksattInnvilget().second.copy(id = behandling.id)


Trenger vi egentlig å matche hele objektet, eller kunne det holde at det er av riktig type f.eks.?

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

christianmosveen · 2022-01-28T09:26:11Z

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

+        )
+        verify(serviceAndMocks.behandlingMetrics).incrementAvslåttCounter(BehandlingMetrics.AvslåttHandlinger.PERSISTERT)
+        verify(serviceAndMocks.ferdigstillVedtakService).lukkOppgaveMedBruker(any())
+        verify(serviceAndMocks.observer).handle(


Tenker det bør være egne tester for at statistikk blir pushet, er ikke viktig for iverksetting.

Men det er jo iverksetting per tidspunkt som sender statistikken (enig i at den bør flyttes sammen med metrikkene til asynk-jobben). Men så lenge iverksett gjør det, forventer jeg at en test feiler dersom jeg sletter den koden.

Enig i det, men jeg tenker testen som da feiler burde vært testen "publiserer statistikk ved iverksetting" e.l. Ideelt sett hadde vi kunnet flytte/endre på statistikk-sideeffekten uten at iverksettingstesten brekker.

christianmosveen · 2022-01-28T09:28:32Z

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

+            ),
+        ) shouldBe expectedAvslag.right()
+
+        verify(serviceAndMocks.søknadsbehandlingRepo).hent(avslagTilAttestering.id)


Tenker med alle verifyene her også, at testen kanskje fortjener et forsøk på et annet navn. Den tester jo nå både at iverksetting blir et avslag, gitt inputen, men også at iverksettingen innebærer at vi henter en behandling, lager et dokument, lagrer en behandling, lagrer et vedtak, lagrer et dokument (av noe slag) og ferdigstiller et vedtak.

Ja, det er en slags fullstendig happy-case, noe jeg egentlig tenker at er greit. Avveiningen her blir litt mellom om man skal ha flere tester med akkurat de samme mocksa, og kun verifies på enkelte features i metoden, eller en som tester hele happycasen

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt

...src/test/kotlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingTestUtils.kt

…andre

...src/test/kotlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingTestUtils.kt

simejo requested a review from a team January 27, 2022 15:00

hestad reviewed Jan 27, 2022

View reviewed changes

hestad mentioned this pull request Jan 27, 2022

Åpne for å avslutte en klage #665

Merged

christianmosveen reviewed Jan 28, 2022

View reviewed changes

simejo force-pushed the tester branch from cc2e9f1 to 883b2eb Compare January 28, 2022 12:23

jacob-meidell reviewed Jan 28, 2022

View reviewed changes

...tlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingServiceIverksettTest.kt Show resolved Hide resolved

jacob-meidell reviewed Jan 28, 2022

View reviewed changes

...src/test/kotlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingTestUtils.kt Show resolved Hide resolved

simejo added 4 commits January 28, 2022 14:42

Endre søknadsbehandlingserviceiverksett-tester

2210e32

Forbedring på tester

2832da5

Feil dersom man kaller en metode som ikke er mocket

34e222b

Omskriving av avkorting-test

d5b7662

simejo force-pushed the tester branch from 883b2eb to d5b7662 Compare January 31, 2022 09:25

simejo added 2 commits January 31, 2022 14:10

Fix avhengigheter til ny defaultMock, og legg tilbake argument-matching

b491bae

Fjerner private variabler for å gjøre testene helt uavhengige av hver…

7e7955f

…andre

hestad approved these changes Jan 31, 2022

View reviewed changes

...src/test/kotlin/no/nav/su/se/bakover/service/søknadsbehandling/SøknadsbehandlingTestUtils.kt Outdated Show resolved Hide resolved

Flyttet spesialisert mock til felles-modulen

0cca61a

simejo merged commit 8b3999f into master Jan 31, 2022

simejo deleted the tester branch January 31, 2022 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Endre søknadsbehandlingserviceiverksett-tester #664

Endre søknadsbehandlingserviceiverksett-tester #664

simejo commented Jan 27, 2022

hestad commented Jan 27, 2022

hestad commented Jan 27, 2022 •

edited

Loading

hestad Jan 27, 2022

simejo Jan 28, 2022

christianmosveen Jan 28, 2022

hestad Jan 28, 2022

christianmosveen Jan 28, 2022

hestad Jan 28, 2022 •

edited

Loading

christianmosveen Jan 28, 2022

hestad Jan 31, 2022

hestad Jan 27, 2022

simejo Jan 28, 2022

hestad Jan 28, 2022

christianmosveen left a comment

christianmosveen Jan 28, 2022

christianmosveen Jan 28, 2022

hestad Jan 28, 2022

christianmosveen Jan 28, 2022

christianmosveen Jan 28, 2022

simejo Jan 31, 2022

Endre søknadsbehandlingserviceiverksett-tester #664

Endre søknadsbehandlingserviceiverksett-tester #664

Conversation

simejo commented Jan 27, 2022

hestad commented Jan 27, 2022

hestad commented Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hestad Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianmosveen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hestad commented Jan 27, 2022 •

edited

Loading

hestad Jan 28, 2022 •

edited

Loading