4
\$\begingroup\$

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)
    }


}
\$\endgroup\$
2
  • \$\begingroup\$ You could run your tests using MockitoExtension rather than SpringExtension if your code is well unit testable. Your tests will run faster. In addtion you may try to not overuse verify \$\endgroup\$
    – eHayik
    Jan 26, 2021 at 16:21
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Feb 10, 2021 at 8:37

2 Answers 2

6
+25
\$\begingroup\$

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,

\$\endgroup\$
7
  • \$\begingroup\$ Good answer in general! But this point "A test is NOT a unit test if:" yields several sometime quite vague sub-points since in doubt it depends on the modularization, orthogonality and granularity level of the testing software component itself whether one should define it as a unit or rather an integration test. In doubt, the software design (and possible flaws here) itself determines the character of several test approaches, they are often not really fully separable. \$\endgroup\$
    – Secundi
    Jan 27, 2021 at 12:28
  • \$\begingroup\$ @Secundi Thank you for reviewing my answer. "A test is NOT a unit test if:" That's the definition by a veteran in the industry. I'm sorry but I can't modify that. The nature of the software test in itself is an integral part in oveall SDLC process so I agree with your thought on idea of software design. The answer does points to the necessary areas where the code and even the design of the software at hand on how it can be improved, the main part of review, and leaves the room for the developer to improve and implement their understanding. But it depends on the developer in the end. \$\endgroup\$ Jan 27, 2021 at 13:43
  • \$\begingroup\$ yes, I'm totally aware of this. Just wanted to mention that quite often, even experienced developers sometimes tend to prioritize focus on the pure testing-side instead of reviewing the own code for a high level of test-compatibility in terms of robust design decisions. Think of the often questionable usage of singletons for instance. In order to achieve a "quite robust" unit testing scheme, often questionable workarounds on the testing side are applied instead of redesigning the actual code that is tested. \$\endgroup\$
    – Secundi
    Jan 27, 2021 at 13:59
  • \$\begingroup\$ Red-Green-Refactor is there for process improvement, that includes the systems design with the test code @Secundi. The purpose of review is a part of that process as well. And those are the basic foundations everyone adheres, experienced or fresher, it does not matter. \$\endgroup\$ Jan 27, 2021 at 14:13
  • \$\begingroup\$ @Anit Shrestha Manandhar thanks for the very detailed answer. This is a legacy application and refactoring isn't really an option at this stage as then we'll have to go through another test cycle etc... Regarding the assert and verify assertions in one test method, is this acceptable given the legacy code? Or am I missing something big/important? \$\endgroup\$ Jan 27, 2021 at 18:05
1
\$\begingroup\$

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.

\$\endgroup\$

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.