Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to specify automatic @Build requirements for unit tests #12

Closed
longwa opened this issue Nov 6, 2013 · 7 comments
Closed

Add a way to specify automatic @Build requirements for unit tests #12

longwa opened this issue Nov 6, 2013 · 7 comments
Assignees

Comments

@longwa
Copy link
Owner

longwa commented Nov 6, 2013

We have some situation where we need to have defaults values for domain objects which include buildLazy() or buildWithoutSave() construction of other domain objects. This is usually to overcome some complicated graph construction or some other issue with circular references, etc.

For example, we might have:

'com.foo.User' {
    someProperty = {-> Group.buildWithoutSave() }
}

This works pretty well and gives us a lot of flexibility. However, it puts a "hidden" requirement on any unit test that needs to build a User. Now they must do:

@Build([User, Group])

But it's not obvious and Group isn't really needed explicitly in the test case.

I'd like to have some way to indicate to the @build annotation that it needs to pull in additional classes, something like:

'com.foo.User' {
    requires([Group, Foo, Bar])
    someProperty = {-> Group.buildWithoutSave() }
}

That way when the user does @build(Unit) they'll get the Group automatically.

What do you think of this approach? I wanted to get some input before doing any work towards it. Our TestDataConfig has gotten large enough in my current project to warrant doing something about it.

@ghost ghost assigned longwa Nov 6, 2013
@tednaleid
Copy link
Collaborator

So this is a unit-test only thing, right? It's necessary for your use case because the way that grails unit tests decorate the domains in a lazy manner doesn't build up the right graph of stuff.

If I'm understanding properly, I'd probably call it something like unitRequires or unitEagerlyLoad to denote that it's only something that affects how unit tests work and doesn't have any impact on things outside of the @Build annotation in unit tests.

Otherwise, this sounds like a better option than the current "hidden" requirement to decorate all unit tests that want a User with the proper dependencies.

One thought that comes to mind...would there be some situations where you'd want one set of domain classes to be built in unit tests, and a different part of the graph (for the same initial domain class)?

Do you have any cases like that in your current code base? I don't have access to any large unit test code bases right now where I could tell if that's a feature I'd want. I guess you could always still include those in the @Build like we do now...

@longwa
Copy link
Owner Author

longwa commented Nov 6, 2013

Yes, this is only a unit test issue. I think being more specific with the configuration makes good sense.

Since I think I read your blog article about this exact same thing, I believe adding a method call into the ConfigSlurper DSL is a bit of a PITA. What if instead we just treated it as a special case attribute? It's a little less pretty but it would make the implementation easier.

'com.foo.User' {
    unitEagerlyLoad = [Group, Foo, Bar]
    someProperty = {-> Group.buildWithoutSave() }
}

I guess the downside is if someone wanted to have an attribute on 'User' called 'unitEagerlyLoad' they wouldn't be able to default it.

@tednaleid
Copy link
Collaborator

How important is it that the eagerly loaded things are in the sampleData stanza of the TestDataConfig.groovy file? I'm wondering if it'd be cleaner if it were in a separate stanza, parallel to the sampleData stanza, maybe something like this:

testDataConfig {
    unitEagerlyLoad {
        'config.Hotel' = ['config.Article', 'Foo', 'Bar']
        'config.Article' = ['config.Hotel']
    }
    sampleData {
        'config.Hotel' {
            name = "Motel 6"
            def i = 6125551111
            faxNumber = {-> "${i++}" } // creates "6125551111", "6125551112", .... 
        }
        'config.Article' {
            def i = 1
            name = {-> "Article ${i++}" }
        }
    }
}

One other thought...would this be transitive/recursive? Meaning that if you had something like this:

unitEagerlyLoad {
    'Foo' = ['Bar']
    'Bar' = ['Baz']
}

and you did a Foo.build(), would you get a Foo only with a Bar, or would that Foo's Bar also have a Baz?

@longwa
Copy link
Owner Author

longwa commented Nov 6, 2013

Separating it makes good sense. I've implemented the style in your last comment. I went ahead and made it resolve a chain of dependencies b/c I think that might be useful for our project. I added some tests to TestDataConfigUnitTests that are passing and I'm going to build and test with our full project before pushing anything.

I just used a map in the config, which makes it easy and fast to do the check (from the bookStore TestDataConfig):

testDataConfig {

    // Whenever we need a hotel, we also need an Article. Allow either String or Class.
    unitEagerlyLoad = [
        'config.Hotel': ['config.Article'],
        'config.Article': [Author]
    ]

    sampleData {

@tednaleid
Copy link
Collaborator

Cool, I think transitive dependencies make sense.

@longwa
Copy link
Owner Author

longwa commented Nov 6, 2013

One small change recommended from co-workers is to call it unitAdditionalBuild instead of unitEagerlyLoad. The consensus was that the word "eager" has too many Grails connotations already. I think I tend to agree.

@tednaleid
Copy link
Collaborator

sure, that makes sense, I wasn't wed to unitEagerlyLoad :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants