-
Notifications
You must be signed in to change notification settings - Fork 96
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
Module Test code to support module setup is correct. #122
Conversation
c26c6f0
to
b6d47d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More documentation needed. I think that will clear up much of my confusion. Also, should this live in the fili-system-config
module, instead of fili-core
, since it seems to be testing system config code?
|
||
import spock.lang.Specification | ||
|
||
abstract class ModuleSetupSpec extends Specification { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description of what this base spec is for and how to use it would be good.
configurations.stream().map( {it.getString(MODULE_NAME_KEY)}).anyMatch({it.equals(getModuleName())}) | ||
} | ||
|
||
abstract String getModuleName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc on this is needed so that usage and contract is clear.
public static final String MODULE_NAME_KEY = "moduleName" | ||
public static final String MODULE_RESOURCE_NAME = "/moduleConfig.properties" | ||
|
||
def "Test that SystemConfig loads for this module"() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- "Test that" isn't helpful in the test name.
- It would be cool if the module name could be injected into here somehow? Perhaps via a
where
block that gets it's value from thegetModuleName
method?
|
||
def "Test that SystemConfig loads for this module"() { | ||
expect: | ||
SystemConfig.properties.getOrDefault("foo", "foo") == "foo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my first test, it tested that the applicationConfig or some other piece of conflictable-config didn't prevent SystemConfig from working
I'm sure there's a better way to do this.
SystemConfig.properties.getOrDefault("foo", "foo") == "foo" | ||
} | ||
|
||
def "Current module is loaded by ConfigResourceLoader"() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to replace "Current module" with an unrolled value from a where
block that could source from the getModuleName()
method.
|
||
def "Current module is loaded by ConfigResourceLoader"() { | ||
setup: | ||
List<Configuration> configurations = new ConfigResourceLoader().loadConfigurations(MODULE_RESOURCE_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should MODULE_RESOURCE_NAME
be fixed or should it be dynamic based on what module is being tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's baked into ModuleLoader as a private constant
List<Configuration> configurations = new ConfigResourceLoader().loadConfigurations(MODULE_RESOURCE_NAME) | ||
|
||
expect: | ||
configurations.stream().map( {it.getString(MODULE_NAME_KEY)}).anyMatch({it.equals(getModuleName())}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing inside the calls is a little inconsistent.
|
||
class BardModuleSetupSpec extends ModuleSetupSpec { | ||
|
||
String getModuleName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs @Override
b6d47d2
to
f739766
Compare
f739766
to
d818e87
Compare
d818e87
to
f3af0a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Though I still think these classes / tests belong in the system-config
module instead of the fili-core
module where the currently reside...
f3af0a3
to
cebe7ae
Compare
* | ||
* @return The name of the module as configured in moduleConfig.properties | ||
*/ | ||
abstract String getModuleName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should move this to higher in the file so that people see this first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good starting point to test modules I guess, we can add more tests later on probably with the module template.
ae5cff1
to
3336ae6
Compare
No description provided.