[ovirt-devel] Why are Java checkstyle checks applied to test code?

[Vojtech Szocs]

----- Original Message -----
> From: "Yevgeny Zaspitsky" <yzaspits at redhat.com>
> To: "Vojtech Szocs" <vszocs at redhat.com>
> Cc: devel at ovirt.org
> Sent: Tuesday, October 7, 2014 2:16:52 PM
> Subject: Re: [ovirt-devel] Why are Java checkstyle checks applied to test code?
> 
> I do not see a difference between "src" and "test" from checkstyle point of
> view.

In general, you are right, but I feel this depends on how (what for) we use
Checkstyle in oVirt Engine project.

There are different kinds of code validation:

a, general "avoid bad coding patterns" validation (i.e. code lint), for this
   we use FindBugs

b, project-specific *general* code style validation (i.e. NewlineAtEndOfFile),
   for this we use Checkstyle

c, project-specific *business* code style validation (i.e. NlsCheck),
   for this we also use Checkstyle

While it makes sense to apply (b) to "test" code, IMHO it makes no sense to
apply (c) to "test" code. For example, checks like NlsCheck (//$NON-NLS-X)
force us to pollute "test" code as it makes no sense to localize strings in
"test" code anyway.

Therefore, we should split (b) and (c) into two separate executions:
- apply (b) for "src" and "test" code
- apply (c) only for "src" code

Each execution would map to separate maven-checkstyle-plugin:check goal
bound to Maven "compile" phase.

> You've encountered a checkstyle problem that might be solved in a less
> drastic way than shutting it down.

The original issue was that Checkstyle's parser failed to parse Java file
if this file contains { and } characters in string, for example:

  private static final String PLACE_HOLDER_STRING = "\\{(\\d+)\\}";

This is internal Checkstyle issue, not an issue with our code. Because
parsing happens *before* applying any Checkstyle checks, suppressing the
problematic portions of Java file will not solve the problem. The only
way to solve this problem is to exclude such files from Checkstyle.

So I have a question, what to do in such cases, i.e. when Checkstyle
fails on some internal (failed to parse, etc.) error, while our code
is perfectly valid? (I don't see "adapting our code to make it parse-
able by Checkstyle" as a proper solution.)

> If that wouldn't a test code you wouldn't
> come up with that solution, right?

As I wrote above, the issue is with Checkstyle, not with our code.
If it happened for "src" code, I'd had to exclude that file from
being analyzed (parsed & processed) by Checkstyle, anyway.

> Have you tried to suppress checkstyle in
> a more local way?
> How about using SuppressionCommentFilter [1] and/or
> @SuppressWarnings("checkstyle:XXX") locally? That [2] is what I found by my
> short googling...

That will not work, see my comments above.

> 
> BTW, as soon as you raised checkstyle topic we might review its
> configuration. E.g. I'm in favor of removing any validation ovirt standard
> formatter does not fix for us automatically or alternatively configure the
> formatter to be such.

+1, I think we should review existing Checkstyle config.

> 
> [1] - http://checkstyle.sourceforge.net/config.html#SuppressionCommentFilter
> [2] -
> http://stackoverflow.com/questions/4023185/how-to-disable-a-particular-checkstyle-rule-for-a-particular-line-of-code
> 
> Best regards,
> ____________________
> Yevgeny Zaspitsky
> 
> 
> 
> ----- Original Message -----
> 
> > From: "Vojtech Szocs" <vszocs at redhat.com>
> > To: devel at ovirt.org
> > Sent: Monday, October 6, 2014 5:25:43 PM
> > Subject: [ovirt-devel] Why are Java checkstyle checks applied to test code?
> 
> > Hi guys,
> 
> > Alex wrote a patch with code in src/test/java containing something like:
> 
> > private static final String PLACE_HOLDER_STRING = "\\{(\\d+)\\}";
> 
> > Checkstyle's parser choked on { and } symbols within the String constant
> > and the whole build failed on Checkstyle "can't parse Java file" error.
> > This is obviously an issue with Checkstyle, not with our Java code.
> 
> > In Engine root POM, we have maven-checkstyle-plugin:check applied to both
> > main *and* test code. So what's the reason/benefit of checking test code
> > with Checkstyle? What to do in cases when Checkstyle parser fails but the
> > given Java class is syntactically OK?
> 
> > Long story short - why this:
> 
> > <includeTestSourceDirectory>true</includeTestSourceDirectory>
> 
> > instead of this?
> 
> > <includeTestSourceDirectory>false</includeTestSourceDirectory>
> 
> > or just removing that config option, since default is false anyway?
> 
> > Reference:
> > http://gerrit.ovirt.org/#/c/32995/2/frontend/webadmin/modules/pom.xml
> 
> > Regards,
> > Vojtech
> > _______________________________________________
> > Devel mailing list
> > Devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/devel
>