Grails

ControllerUnitTestCase does not populate model unless view is explicitly set

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Not A Bug
  • Affects Version/s: 1.2 final
  • Fix Version/s: 1.2.1
  • Component/s: Testing
  • Labels:
    None
  • Environment:
    Windows XP, Java 1.6.0_17

Description

I have this method in a controller:

def show = {
def profile
if (params.username) { profile = profileService.getPublicProfileForUser(params.username) }
if (!profile) { response.sendError(404) return; }
[profile: profile]
}

The method works when browsing on the site following grails run-app

I've also written a unit test extending ControllerUnitTestCase:

void testShowWithUser() {
def profileServiceControl = mockFor(ProfileService)
profileServiceControl.demand.getPublicProfileForUser(1) { new PublicProfile() }
controller.profileService = profileServiceControl.createMock()

controller.params.username = 'viewedUser'

controller.show()

assertEquals "Wrong view", "show", controller.modelAndView.view
assertNotNull "Missing profile in returned model", controller.modelAndView.model.profile
profileServiceControl.verify()

}

The test fails for both assertions after grails test-app, ie the view name is not set and the model is not populated.

Changing the last line of the controller show() method to the following allows the test to pass:

render view: "show", model: [profile: profile]

So it's not possible to test model and view without explicitly setting the view in a render statement.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jeff Brown added a comment -

I think that your test should do something like this...

test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        def profileServiceControl = mockFor(ProfileService)
        profileServiceControl.demand.getPublicProfileForUser(1) { new PublicProfile() }
        controller.profileService = profileServiceControl.createMock()

        controller.params.username = 'viewedUser'

        def model = controller.show()
        assertNotNull "Missing profile in returned model", model.profile
        profileServiceControl.verify()

    }
}

That will assert that a model was returned (render was not called) and that the model contains a profile.

Is that what your test should be doing?

On an unrelated note... in your unit test you don't really need to mock out your ProfileService, you could just plug in a fake like a Map if you wanted. I don't think that is relevant to your real question though...

test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        controller.profileService = [getPublicProfileForUser: { 'dummy user'}]

        controller.params.username = 'viewedUser'

        def model = controller.show()
        
        // just make sure the model includes whatever the profileService returned...
        assertEquals 'Wrong profile in returned model', 'dummy user', model.profile
    }
}
Show
Jeff Brown added a comment - I think that your test should do something like this...
test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        def profileServiceControl = mockFor(ProfileService)
        profileServiceControl.demand.getPublicProfileForUser(1) { new PublicProfile() }
        controller.profileService = profileServiceControl.createMock()

        controller.params.username = 'viewedUser'

        def model = controller.show()
        assertNotNull "Missing profile in returned model", model.profile
        profileServiceControl.verify()

    }
}
That will assert that a model was returned (render was not called) and that the model contains a profile. Is that what your test should be doing? On an unrelated note... in your unit test you don't really need to mock out your ProfileService, you could just plug in a fake like a Map if you wanted. I don't think that is relevant to your real question though...
test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        controller.profileService = [getPublicProfileForUser: { 'dummy user'}]

        controller.params.username = 'viewedUser'

        def model = controller.show()
        
        // just make sure the model includes whatever the profileService returned...
        assertEquals 'Wrong profile in returned model', 'dummy user', model.profile
    }
}
Hide
Permalink
Dave Bower added a comment -

Writing model = controller.show() certainly works for getting the model, but what about the view? Or is that the "right way" to do it? My assumption was that using modelAndView property was the way it was done?

Good point about the mocking, thanks.

D.

Show
Dave Bower added a comment - Writing model = controller.show() certainly works for getting the model, but what about the view? Or is that the "right way" to do it? My assumption was that using modelAndView property was the way it was done? Good point about the mocking, thanks. D.
Hide
Permalink
Jeff Brown added a comment -

If the controller action returns a model (which your test is asserting) then the view is going to be the name of the controller action. There is nothing to assert there in a unit test. Your unit test should be focused on the logic that is inside of the unit being tested. In your case, there is no logic in your controller action that dictates a view to render.

Show
Jeff Brown added a comment - If the controller action returns a model (which your test is asserting) then the view is going to be the name of the controller action. There is nothing to assert there in a unit test. Your unit test should be focused on the logic that is inside of the unit being tested. In your case, there is no logic in your controller action that dictates a view to render.
Hide
Permalink
Dave Bower added a comment -

Say my controller changes to have some logic to return a view, instead of a 404, in that case I would get benefit from asserting that the correct view has been rendered in both cases, so what then? Is the right thing to check for a null modelAndView and then I know everything's worked as that seems a little loose, not really a positive test? - It wouldn't catch any refactor errors such as renaming the controller method without changing the gsp.

I understand that you think the test is checking the framework and not the logic but I would suggest that a positive check that the correct view is being rendered is very useful, whatever the default framework behaviour is.

Is the modelAndView property working in the mock as it does in the running app? If so I guess I can't expect it to work any other way.

Show
Dave Bower added a comment - Say my controller changes to have some logic to return a view, instead of a 404, in that case I would get benefit from asserting that the correct view has been rendered in both cases, so what then? Is the right thing to check for a null modelAndView and then I know everything's worked as that seems a little loose, not really a positive test? - It wouldn't catch any refactor errors such as renaming the controller method without changing the gsp. I understand that you think the test is checking the framework and not the logic but I would suggest that a positive check that the correct view is being rendered is very useful, whatever the default framework behaviour is. Is the modelAndView property working in the mock as it does in the running app? If so I guess I can't expect it to work any other way.
Hide
Permalink
Jeff Brown added a comment -

Right now you controller looks something like this...

grails-app/controllers/DemoController.groovy
class DemoController {
    def profileService
    def show = {
        def profile
        if (params.username) { 
            profile = profileService.getPublicProfileForUser(params.username) 
        }
        if (!profile) { 
            response.sendError(404) 
            return; 
        }
        [profile: profile]
    }
}

One thing that you might want to test is that when a username parameter is passed then the controller is supposed to return a model and the model should contain the user retrieved from the service. That test could look something like this:

test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        controller.profileService = [getPublicProfileForUser: { 'dummy user'}]

        controller.params.username = 'viewedUser'

        def model = controller.show()
        
        // just make sure the model includes whatever the profileService returned...
        assertEquals 'Wrong profile in returned model', 'dummy user', model.profile
    }
}

You asked about what to do if you changed your controller logic to render some view instead of returning a 404 if no username request parameter is supplied. That scenario does not effect the requirement described above. That is all still valid. The handling of the scenario where no username is specified is a different scenario and calls for a different test. You might write a test that looks something like this:

test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        controller.profileService = [getPublicProfileForUser: { 'dummy user'}]

        controller.params.username = 'viewedUser'

        def model = controller.show()

        // just make sure the model includes whatever the profileService returned...
        assertEquals 'Wrong profile in returned model', 'dummy user', model.profile
    }

    void testShowWithoutUser() {
        def model = controller.show()

        // make sure the correct view is rendered
        assertEquals 'wrong view was rendered', 'missingUserErrorPage', controller.modelAndView.view
    }
}

That test will fail because the controller is returning a 404, not rendering a view (TDD at work). Then you could update your controller with something like this...

grails-app/controllers/DemoController.groovy
class DemoController {
    def profileService
    def show = {
        def profile
        if (params.username) { 
            profile = profileService.getPublicProfileForUser(params.username) 
        }
        if (!profile) { 
            render view: 'missingUserErrorPage'
        } else {
            return [profile: profile]
        }
    }
}

The testShowWithUser is making sure a model is returned. The testShowWIthoutUser is making sure a view was rendered. Each is testing the requirement relevant to a specific scenario (username supplied or not supplied).

Show
Jeff Brown added a comment - Right now you controller looks something like this...
grails-app/controllers/DemoController.groovy
class DemoController {
    def profileService
    def show = {
        def profile
        if (params.username) { 
            profile = profileService.getPublicProfileForUser(params.username) 
        }
        if (!profile) { 
            response.sendError(404) 
            return; 
        }
        [profile: profile]
    }
}
One thing that you might want to test is that when a username parameter is passed then the controller is supposed to return a model and the model should contain the user retrieved from the service. That test could look something like this:
test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        controller.profileService = [getPublicProfileForUser: { 'dummy user'}]

        controller.params.username = 'viewedUser'

        def model = controller.show()
        
        // just make sure the model includes whatever the profileService returned...
        assertEquals 'Wrong profile in returned model', 'dummy user', model.profile
    }
}
You asked about what to do if you changed your controller logic to render some view instead of returning a 404 if no username request parameter is supplied. That scenario does not effect the requirement described above. That is all still valid. The handling of the scenario where no username is specified is a different scenario and calls for a different test. You might write a test that looks something like this:
test/unit/DemoControllerTests.groovy
import grails.test.*

class DemoControllerTests extends ControllerUnitTestCase {

    void testShowWithUser() {
        controller.profileService = [getPublicProfileForUser: { 'dummy user'}]

        controller.params.username = 'viewedUser'

        def model = controller.show()

        // just make sure the model includes whatever the profileService returned...
        assertEquals 'Wrong profile in returned model', 'dummy user', model.profile
    }

    void testShowWithoutUser() {
        def model = controller.show()

        // make sure the correct view is rendered
        assertEquals 'wrong view was rendered', 'missingUserErrorPage', controller.modelAndView.view
    }
}
That test will fail because the controller is returning a 404, not rendering a view (TDD at work). Then you could update your controller with something like this...
grails-app/controllers/DemoController.groovy
class DemoController {
    def profileService
    def show = {
        def profile
        if (params.username) { 
            profile = profileService.getPublicProfileForUser(params.username) 
        }
        if (!profile) { 
            render view: 'missingUserErrorPage'
        } else {
            return [profile: profile]
        }
    }
}
The testShowWithUser is making sure a model is returned. The testShowWIthoutUser is making sure a view was rendered. Each is testing the requirement relevant to a specific scenario (username supplied or not supplied).
Hide
Permalink
Jeff Brown added a comment -

Your comment included "It wouldn't catch any refactor errors such as renaming the controller method without changing the gsp.". A unit test of a controller should not be concerned with the name or existence of a gsp. The unit test for your controller should focus on the logic that is inside of your controller. If a gsp is missing or misnamed, that is a problem for a different kind of test.

Show
Jeff Brown added a comment - Your comment included "It wouldn't catch any refactor errors such as renaming the controller method without changing the gsp.". A unit test of a controller should not be concerned with the name or existence of a gsp. The unit test for your controller should focus on the logic that is inside of your controller. If a gsp is missing or misnamed, that is a problem for a different kind of test.
Hide
Permalink
Dave Bower added a comment - - edited

Thanks Jeff, going back to the subject of the bug, should the modelAndView property be set when the view is the default? If not then just close this issue, but if it should then I guess it still needs fixing, though it can be worked around as you suggest.

Show
Dave Bower added a comment - - edited Thanks Jeff, going back to the subject of the bug, should the modelAndView property be set when the view is the default? If not then just close this issue, but if it should then I guess it still needs fixing, though it can be worked around as you suggest.
Hide
Permalink
Jeff Brown added a comment -

I think the test case is working as designed and there are good techniques for testing the kinds of things discussed here.

Thanks for the input.

Show
Jeff Brown added a comment - I think the test case is working as designed and there are good techniques for testing the kinds of things discussed here. Thanks for the input.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.1#660-r161644) | Report a problem
Powered by a free Atlassian JIRA open source license for Grails project. Try JIRA - bug tracking software for your team.