Are these Unit tests OK or am I implementing an antipattern or not following best practices

Question

I am testing a Spring boot service using JUnit 5 and Mockito. I haven't spent much time on unit testing in the past so I'm not sure if I'm accidentally implementing an anti-pattern or not following best practices.

I read multiple times that it's good practice to have one assertion per test, I have one assertion but I also have a few verify assertions that check if the method I'm testing called the correct methods with the correct input based on the conditional branch I'm testing as well as the input to the method I'm testing, is this OK/acceptable or should I split it up into one assertion/verify assertion per test? It feels like that would require a lot of code duplication... Below are four tests I wrote and the method I'm testing. For clarity I'll also add the code that sets up and handles my test application context.

Any ideas/advice would be much appreciated.

The method I am testing:

@Transactional
    override fun startSession(msisdn: String, origConnID: String, configSetName: String): StartSessionResponseDTO {
        val txId = generateTxID()
        val ss = sessionStatus(msisdn)

        eventService.logEvent(EventType.StartSessionRequest, txId, msisdn, hashMapOf("extTxId" to origConnID))

        if (ss == null) {
            throw ApplicationException(type = ApplicationException.Type.client_error, message = "Concurrent Access Detected: ${msisdn}")
        } else {
            if (ss.sessionId == null || ss.ip == null) {
                //create new Nuance session
                nuanceService.startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName)).let {
                    updateSession(msisdn = msisdn, sessionId = it.sessionId, ip = it.ip, origConnID = origConnID)
                }
            } else {
                //check if the sessionId is still valid
                if (!nuanceService.isSessionValid(txId = txId, msisdn = msisdn, sessionId = ss.sessionId, host = ss.ip, configSetName = configSetName)) {
                    nuanceService.startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName)).let {
                        updateSession(msisdn = msisdn, sessionId = it.sessionId, ip = it.ip, origConnID = origConnID)
                    }
                }
            }
        }

        eventService.logEvent(type = EventType.StartSessionResponse, txId = txId, msisdn = msisdn)
        return StartSessionResponseDTO(msisdn)
    }

My test class:

@ExtendWith(SpringExtension::class)
@ContextConfiguration()
class  VBServiceTests() {

    @TestConfiguration
    class testConfig {
        @Bean
        fun jdbcTemplate(): NamedParameterJdbcTemplate {
            return mock<NamedParameterJdbcTemplate>()
        }

        @Bean
        fun nuanceService(): NuanceService {
            return mock<NuanceService>()
        }

        @Bean
        fun appConfigProps(): AppConfigProps {
            return mock<AppConfigProps>()
        }

        @Bean
        fun eventService(): EventServiceImp {
            return mock<EventServiceImp>()
        }

        @Bean
        fun audioTrimService(): AudioTrimService {
            return mock<AudioTrimService>()
        }

        @Bean
        fun vbNuanceStagingDeletionService(): VbNuanceStagingDeletionsService {
            return mock<VbNuanceStagingDeletionsService>()
        }
    }

    @MockBean
    lateinit var nuanceService: NuanceService

    @MockBean
    lateinit var eventService: EventServiceImp

    @SpyBean
    lateinit var vbServiceSpy: VbServiceImp

    val msisdn = "0821234567"
    val origConnID = "o123"
    val sessionId = "0821234567"
    val ip = "127.0.0.1"
    val txId = "1234-5678"
    val configSetName = "LoIvrPhraIvrHdr"

    @Test
    fun `startSession | When method is called with valid input a StartSessionDTO is returned, if no Nuance session exists a new Nuance session will be created and persisted`() {
        doNothing().whenever(vbServiceSpy).updateSession(msisdn, origConnID, sessionId, ip)
        doReturn(txId).whenever(vbServiceSpy).generateTxID()
        doReturn(SessionStatus(msisdn, origConnID, null, null)).whenever(vbServiceSpy).sessionStatus(msisdn)

        given(nuanceService.startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName))).willReturn(NuanceStartSessionRespDto(txId, sessionId, ip))

        assertThat(vbServiceSpy.startSession(msisdn, origConnID, configSetName), Is(StartSessionResponseDTO(msisdn)))
        verify(vbServiceSpy).generateTxID()
        verify(vbServiceSpy).sessionStatus(msisdn)
        verify(eventService).logEvent(StartSessionRequest, txId, msisdn, hashMapOf("extTxId" to origConnID))
        verify(nuanceService).startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName))
        verify(vbServiceSpy).updateSession(msisdn = msisdn, sessionId = sessionId, ip = ip, origConnID = origConnID)
        verify(eventService).logEvent(type = EventType.StartSessionResponse, txId = txId, msisdn = msisdn)
    }

    @Test
    fun `startSession | When method is called with valid input a StartSessionDTO is returned, if existing valid Nuance session exists the existing session will be used`() {
        doNothing().whenever(vbServiceSpy).updateSession(msisdn, origConnID, sessionId, ip)
        doReturn(txId).whenever(vbServiceSpy).generateTxID()
        doReturn(SessionStatus(msisdn, origConnID, sessionId, ip)).whenever(vbServiceSpy).sessionStatus(msisdn)

        given(nuanceService.isSessionValid(txId, msisdn, sessionId, ip, configSetName)).willReturn(true)

        assertThat(vbServiceSpy.startSession(msisdn, origConnID, configSetName), Is(StartSessionResponseDTO(msisdn)))
        verify(vbServiceSpy).generateTxID()
        verify(vbServiceSpy).sessionStatus(msisdn)
        verify(eventService).logEvent(StartSessionRequest, txId, msisdn, hashMapOf("extTxId" to origConnID))
        verify(eventService).logEvent(type = EventType.StartSessionResponse, txId = txId, msisdn = msisdn)
    }

    @Test
    fun `startSession | When method is called with valid input a StartSessionDTO is returned, if existing invalid Nuance session exists a new session will be created and the existing session record will be updated`() {
        doNothing().whenever(vbServiceSpy).updateSession(msisdn, origConnID, sessionId, ip)
        doReturn(txId).whenever(vbServiceSpy).generateTxID()
        doReturn(SessionStatus(msisdn, origConnID, sessionId, ip)).whenever(vbServiceSpy).sessionStatus(msisdn)

        given(nuanceService.isSessionValid(txId, msisdn, sessionId, ip, configSetName)).willReturn(false)
        given(nuanceService.startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName))).willReturn(NuanceStartSessionRespDto(txId, sessionId, ip))

        assertThat(vbServiceSpy.startSession(msisdn, origConnID, configSetName), Is(StartSessionResponseDTO(msisdn)))
        verify(vbServiceSpy).generateTxID()
        verify(vbServiceSpy).sessionStatus(msisdn)
        verify(eventService).logEvent(StartSessionRequest, txId, msisdn, hashMapOf("extTxId" to origConnID))
        verify(nuanceService).startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName))
        verify(vbServiceSpy).updateSession(msisdn = msisdn, sessionId = sessionId, ip = ip, origConnID = origConnID)
        verify(eventService).logEvent(type = EventType.StartSessionResponse, txId = txId, msisdn = msisdn)
    }

    @Test()
    fun `startSession - When method is called with valid input when sessionStatus returns null concurrent access is detected and an ApplicationException gets thrown`() {
        doReturn(txId).whenever(vbServiceSpy).generateTxID()
        doReturn(null).whenever(vbServiceSpy).sessionStatus(msisdn)

        val exception = Assertions.assertThrows(ApplicationException::class.java) {
            vbServiceSpy.startSession(msisdn, origConnID, configSetName)
        }

        assertThat(exception.message, Is("Concurrent Access Detected: ${msisdn}"))


        verify(vbServiceSpy).generateTxID()
        verify(vbServiceSpy).sessionStatus(msisdn)
    }


}

Answer 1

Some Concepts

I'm not much experienced in the framework used but Unit Testing, Test Driven Development and Behavioral Driven Development are concepts that need to be understood well and then implemented properly.

Unit Test Definition by Michael Feathers

I've used these rules with a large number of teams. They encourage good design and rapid feedback and they seem to help teams avoid a lot of trouble.

A test is NOT a unit test if:

• It talks to the database
• It communicates across the network
• It touches the file system
• It can't run correctly at the same time as any of your other unit tests
• You have to do special things to your environment (such as editing config files) to run it.

Tests that do things things aren't bad. Often they are worth writing, and they can be written in a unit test harness. However, it is important to be able to separate them from true unit tests so that we can keep a set of tests that we can run fast whenever we make our changes.

---

The Three Laws of TDD by Robert Martin

• First Law : You may not write production code until you have written a failing unit test.

• Second Law : You may not write more of a unit test than is sufficient to fail, and not compiling is failing.

• Third Law : You may not write more production code than is sufficient to pass the currently failing test.

Test-driven development (TDD) is a software development process that relies on the repetition of a very short development cycle: requirements are turned into very specific test cases, then the software is improved to pass the new tests, only. This is opposed to software development that allows software to be added that is not proven to meet requirements. - Wiki

---

BDD

Behavior-Driven Development is a software development process that puts feature behaviors first. A behavior is how a feature operates within a well-defined scenario of inputs, actions, and outcomes. Behaviors are identified using specification by example. Behavior specs become the requirements, the acceptance criteria, and the acceptance tests. Test frameworks can directly automate specs as well – declarative specs for unique product behaviors should be the units of coverage. The most prevalent BDD test frameworks are Cucumber derivatives that write specs in the “Given-When-Then” Gherkin language.

Given steps should use past or present-perfect tense, because they represent an initial state that must already be established.
When steps should use present tense, because they represent actions actively performed as part of the behavior.
Then steps should use present or future tense, because they represent what should happen after the behavior actions.

Good example, clear, short and precisely does check one behavior.

Feature: Google Searching

Scenario: Search result linking

  Given Google search results for "panda" are shown
  When the user clicks the first result link
  Then the page for the chosen result link is displayed

---

Red Green Refactor by James Shore

• Think: Figure out what test will best move your code towards completion. (Take as much time as you need. This is the hardest step for beginners.)

• Red: Write a very small amount of test code. Only a few lines... usually no more than five. Run the tests and watch the new test fail: the test bar should turn red. (This should only take about 30 seconds.)

• Green: Write a very small amount of production code. Again, usually no more than five lines of code. Don't worry about design purity or conceptual elegance. Sometimes you can just hardcode the answer. This is okay because you'll be refactoring in a moment. Run the tests and watch them pass: the test bar will turn green. (This should only take about 30 seconds, too.)

• Refactor: Now that your tests are passing, you can make changes without worrying about breaking anything. Pause for a moment. Take a deep breath if you need to. Then look at the code you've written, and ask yourself if you can improve it. Look for duplication and other "code smells." If you see something that doesn't look right, but you're not sure how to fix it, that's okay. Take a look at it again after you've gone through the cycle a few more times. (Take as much time as you need on this step.) After each little refactoring, run the tests and make sure they still pass.

• Repeat: Do it again. You'll repeat this cycle dozens of times in an hour. Typically, you'll run through several cycles (three to five) very quickly, then find yourself slowing down and spending more time on refactoring. Than you'll speed up again. 20-40 cycles in an hour is not unreasonable.

This process works well for two reasons. First, you're working in baby steps, constantly forming hypotheses and checking them. ("The bar should turn red now... now it should turn green... now it should still be green... now it should be red...") Whenever you make a mistake, you catch it right away. It's only been a few lines of code since you made the mistake, which makes the mistake very easy to find and fix. We all know that finding mistakes, not fixing them, is the most expensive part of programming.

The other reason this process works well is that you're always thinking about design. Either you're deciding which test you're going to write next, which is an interface design process, or you're deciding how to refactor, which is a code design process. All of this thought on design is immediately tested by turning it into code, which very quickly shows you if the design is good or bad.

---

Reasons to Use Mocks by Baeldung

We’ll start assuming that you already code following some driven development methodology centered on tests (TDD, ATDD or BDD). Or simply that you want to create a test for an existing class that relies on dependencies to achieve its functionality.

In any case, when unit-testing a class, we want to test only its functionality and not that of its dependencies (either because we trust their implementation or because we’ll test it ourselves).

To achieve this, we need to provide to the object-under-test, a replacement that we can control for that dependency. This way we can force extreme return values, exception throwing or simply reduce time-consuming methods to a fixed return value.

This controlled replacement is the mock, and it will help you to simplify test coding and to reduce test execution time.

---

Function Retrospect

@Transactional
    override fun startSession(msisdn: String, origConnID: String, configSetName: String): StartSessionResponseDTO {
        
        val txId = generateTxID()
        val ss = sessionStatus(msisdn)

        eventService.logEvent(EventType.StartSessionRequest, txId, msisdn, hashMapOf("extTxId" to origConnID))

        if (ss == null) {
            throw ApplicationException(type = ApplicationException.Type.client_error, message = "Concurrent Access Detected: ${msisdn}")
        } else {
            if (ss.sessionId == null || ss.ip == null) {
                //create new Nuance session
                nuanceService.startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName)).let {
                    updateSession(msisdn = msisdn, sessionId = it.sessionId, ip = it.ip, origConnID = origConnID)
                }
            } else {
                //check if the sessionId is still valid
                if (!nuanceService.isSessionValid(txId = txId, msisdn = msisdn, sessionId = ss.sessionId, host = ss.ip, configSetName = configSetName)) {
                    nuanceService.startSession(NuanceStartSessionReqDto(txId, msisdn, configSetName = configSetName)).let {
                        updateSession(msisdn = msisdn, sessionId = it.sessionId, ip = it.ip, origConnID = origConnID)
                    }
                }
            }
        }

        eventService.logEvent(type = EventType.StartSessionResponse, txId = txId, msisdn = msisdn)
        return StartSessionResponseDTO(msisdn)
    }

Probable Code Modifications

• This function has too many if else condition. Probably break those into smaller units so that those can be Unit tested.

• As the function is just related to starting session, Transaction ID generation dependency could be moved out of the functions scope, and passed as an input.

• I can see a Session Class which has the variables those passes in the startSession().

  Class Session { msisdn: String origConnID: String configSetName: String transactionID: String sessionStatus: boolean

  public Session( ALL VARIABLES ) {
    // INITIALIZE ALL VARIABLES 
  } 
  

  }

I think startSession() is doing a lot of things. Divide and conquer. The other purpose of writing test is also to find smaller functionalities which might be independent from other. With the logic, it also gives us the opportunity to check whether the datastructure and algorithms implemnted were proper previously. So the more test cases we create, cleaner our thought about what exactly the function should be doing.

---

Behavior Test Retrospect

From the first glance, the scenarios are not clear as thedefinitions are too long.

fun startSession | When method is called with valid input a StartSessionDTO is returned, if no Nuance session exists a new Nuance session will be created and persisted() { }

TO

fun Starts session successfully when no Nuance session exists

fun Session fails to start when msisdn exists

fun Session fails to start when Nuance session exists

We want our test to complete fast and fail fast. The comparision of object is never a good idea. So it would be better to assert values as much as possible. Further more the number of assertions should not be more as more assertions indicates that the function is doing a lot of things, and the verification of some of those assestions needs separate function and their test.

There seems to be just one of data, probably because of one scenario. It would be better to have as much varied data as possible as the data represents the test cases.

---

Understanding of CLEAN code and SOLID principle would be much helpful.

I hope the explanation is not too much long and useful to you.

Cheers,

Answer 2

One Assertion

I try to make sure that each of my tests is only testing one thing. However, that's not the same as there only being one assertion. Consider a method that takes in a list and returns a sorted version of the list. If I pass in an unsorted list of two items, then I can check that the correct sorted list has been returned by ensuring that the returned list has two items, item one has the correct value and item two has the correct value. That's three assertions, so it could be three tests. However you could take the view that the assertions are all related, so you only need one test. My preferred assertion framework AssertJ provides a helper method (containsExactly), so this can by a single assertion. Try to include only what's important to the test and not the bits that aren't.

Verify

doReturn(null).whenever(vbServiceSpy).sessionStatus(msisdn)
....
verify(vbServiceSpy).sessionStatus(msisdn)

I consider this to be a bit of an anti-pattern. The whenever call knows more about it's arguments than it really needs to know, which sets up unnecessary duplication of knowledge in your tests and makes them more brittle than they need to be. When you are setting up your doReturns, consider if you really need to know about the argument being supplied when deciding what to return. This can be the case when you need to have different return values for different parameters, however most of the time you can get away with using the any() matcher.

doReturn(null).whenever(vbServiceSpy).sessionStatus(any())

You already validate the correct value is being supplied to the call with your verify, you don't need to restrict the behaviour of your stub as well.

Duplication

When I'm structuring my tests, I try to avoid duplication of knowledge, rather than duplication of code. So, looking at your tests, all of them contain:

verify(vbServiceSpy).generateTxID()

To me, this is its own test (startSession generates a new transaction ID). Once you have that test, the other tests no longer need to explicitly verify it, they can assume it's tested elsewhere.

All of you tests also have the corresponding:

doReturn(txId).whenever(vbServiceSpy).generateTxID()

Some of them care about the value txId, because they use it elsewhere in the test, however some of them like 'when method is called with invalid input' doesn't. Consider putting some of these repeated stubbings into a BeforeTest method. By using any for parameter matching and returning sensible defaults you can often remove a lot of noise from your test setups and the tests can focus on the individual sections of code that they are testing.

So, you might have a test that validated that the generated transaction id is logged. The relevant bits from your tests would appear to be:

// Setup the service to return a known transaction id
doReturn("1111-9999").whenever(vbServiceSpy).generateTxID()

// Invoke the code under test.  Notice we don't need to assert anything
// on the return value because for this test, we don't care what's returned
// other tests care about that.
vbServiceSpy.startSession(msisdn, origConnID, configSetName)

// Verify that the known transaction id was passed to the log
// with StartSessionRequest.  This test doesn't care about the other
// arguments.
verify(eventService).logEvent(eq(StartSessionRequest), eq("1111-9999"), any(), any())

That's a three line test (assuming the BeforeTest has been stubbed correctly), that validates that the new transaction id is always logged when the method is called. You then don't need to worry about those bits in your other tests. At the moment you're testing this logging in 3 out of your 4 tests, even though the log takes place in all 4 of your scenarios, so there is a bit of a whole in your coverage, albeit one that you probably don't care about.