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

AuditEventDefaultFormatterTest should not use PowerMockito for testing #4931

Open
romani opened this Issue Aug 10, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Aug 10, 2017

from 0dc6ed7#r131657399

AuditEventDefaultFormatterTest can create AuditEvent class and fill it with all required values for testing. Mocks are not required here.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 10, 2017

Member

@romani I started looking into this and mocks may be required for 2 of the tests.

testFormatModuleNameContainsCheckSuffix mocks the source name as TestModuleCheck. Source name comes from LocalizedMessage which gets the value from the actual Class object given to it.
The only way we can duplicate this without a mock is if the class we give it is in the base package, IE directly in src/test/java.

Without these cases, we lose coverage in AuditEventDefaultFormatter.getCheckShortName for the true condition of if (lastDotIndex == -1) {.

If you are fine with putting classes into src/test/java, then we can do that.

Member

rnveach commented Aug 10, 2017

@romani I started looking into this and mocks may be required for 2 of the tests.

testFormatModuleNameContainsCheckSuffix mocks the source name as TestModuleCheck. Source name comes from LocalizedMessage which gets the value from the actual Class object given to it.
The only way we can duplicate this without a mock is if the class we give it is in the base package, IE directly in src/test/java.

Without these cases, we lose coverage in AuditEventDefaultFormatter.getCheckShortName for the true condition of if (lastDotIndex == -1) {.

If you are fine with putting classes into src/test/java, then we can do that.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 18, 2017

Member

@rnveach ,

I started looking into this and mocks may be required for 2 of the tests.

I fine with mocks, till it is reasonable. I will be good to have a rule that demand javadoc over test method if test is using mock. Engineers hate javadoc writing, so it will force them to think about more functional test, and it is ease to catch in codereview.
Meanwhile let update code that is easy to update and keep mocks in complicated cases only with explanation like "argument is Class type, that is final , no way to easily mock it so we use powermock".

directly in src/test/java

I do not like this.

we lose coverage in

we need coverage as coverage help more that a bit weird code.

Source name comes from LocalizedMessage which gets the value from the actual Class object given to it.

it is a sign of design problem that API class takes a Class as argument by in real life use only String identifier from it. This argument should be a of String type and probably completely deprecated in favor of moduleId.
If you agree lets make this this API change a separate issue.

Member

romani commented Aug 18, 2017

@rnveach ,

I started looking into this and mocks may be required for 2 of the tests.

I fine with mocks, till it is reasonable. I will be good to have a rule that demand javadoc over test method if test is using mock. Engineers hate javadoc writing, so it will force them to think about more functional test, and it is ease to catch in codereview.
Meanwhile let update code that is easy to update and keep mocks in complicated cases only with explanation like "argument is Class type, that is final , no way to easily mock it so we use powermock".

directly in src/test/java

I do not like this.

we lose coverage in

we need coverage as coverage help more that a bit weird code.

Source name comes from LocalizedMessage which gets the value from the actual Class object given to it.

it is a sign of design problem that API class takes a Class as argument by in real life use only String identifier from it. This argument should be a of String type and probably completely deprecated in favor of moduleId.
If you agree lets make this this API change a separate issue.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 19, 2017

Member

a sign of design problem that API class takes a Class as argument by in real life use only String identifier from it

It is only used by us for a String. AuditEvents get sent to listeners, so they could want the class object for some reason, like if they want to know who is sending the message.
I assume you don't want to keep both class object and module name/id but rely on the name/id more?

This argument should be a of String type and probably completely deprecated in favor of moduleId.

Module ID is only set if the module has an ID, so we would need to change more code to swap between module name and ID depending on which is set.

Member

rnveach commented Aug 19, 2017

a sign of design problem that API class takes a Class as argument by in real life use only String identifier from it

It is only used by us for a String. AuditEvents get sent to listeners, so they could want the class object for some reason, like if they want to know who is sending the message.
I assume you don't want to keep both class object and module name/id but rely on the name/id more?

This argument should be a of String type and probably completely deprecated in favor of moduleId.

Module ID is only set if the module has an ID, so we would need to change more code to swap between module name and ID depending on which is set.

romani added a commit that referenced this issue Aug 19, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 19, 2017

Member

AuditEvents get sent to listeners, so they could want the class object for some reason, like if they want to know who is sending the message.

"who sending message" is identifier, identifier should be a string as more flexible data type. String could contain simple name of class or some ID, or some formatted string or ..... .

I assume you don't want to keep both class object and module name/id but rely on the name/id more?

yes, we could even go further eventually and keep only id field. Identification filed should be alone it is enough.

Module ID is only set if the module has an ID, so we would need to change more code to swap between module name and ID depending on which is set.

yes.

Member

romani commented Aug 19, 2017

AuditEvents get sent to listeners, so they could want the class object for some reason, like if they want to know who is sending the message.

"who sending message" is identifier, identifier should be a string as more flexible data type. String could contain simple name of class or some ID, or some formatted string or ..... .

I assume you don't want to keep both class object and module name/id but rely on the name/id more?

yes, we could even go further eventually and keep only id field. Identification filed should be alone it is enough.

Module ID is only set if the module has an ID, so we would need to change more code to swap between module name and ID depending on which is set.

yes.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 19, 2017

Member

String could contain simple name of class or some ID, or some formatted string or ..... .

I was referring to a custom listener may want the physical class, and we only give him simple name/ID.

Let's create a new issue.
We can close this one, right?

Member

rnveach commented Aug 19, 2017

String could contain simple name of class or some ID, or some formatted string or ..... .

I was referring to a custom listener may want the physical class, and we only give him simple name/ID.

Let's create a new issue.
We can close this one, right?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 19, 2017

Member

We can close this one, right?

do you agree to make rule to put comment on methods that can not be done withtout mocks?
There is still one test method I think we need to left a comment why it use mocks.

Member

romani commented Aug 19, 2017

We can close this one, right?

do you agree to make rule to put comment on methods that can not be done withtout mocks?
There is still one test method I think we need to left a comment why it use mocks.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 19, 2017

Member

I am fine with this rule. Should we make a new issue for all mocks we have?

Member

rnveach commented Aug 19, 2017

I am fine with this rule. Should we make a new issue for all mocks we have?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 19, 2017

Member

Let's create a new issue.

#4969

Should we make a new issue for all mocks we have?

lets create, till it not automated, lets try to demand this in PRs.

Member

romani commented Aug 19, 2017

Let's create a new issue.

#4969

Should we make a new issue for all mocks we have?

lets create, till it not automated, lets try to demand this in PRs.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 19, 2017

Member

lets create, till it not automated, lets try to demand this in PRs.

#4970

Member

rnveach commented Aug 19, 2017

lets create, till it not automated, lets try to demand this in PRs.

#4970

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

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