#267 SeverityMatchFilter doesn't work w/ level != info

release_4.0
closed
Oliver Burn
5
2012-10-10
2004-12-07
Henri Tremblay
No

If you launch the ant task with for instance:

<module name="SeverityMatchFilter">
   <property name="severity" value="error"/>
   <property name="acceptOnMatch" value="true"/>
</module>

The output from the xml logger will be wrong. It will
look like that:

<error line="7" severity="error" message="Line does not match expected header line of ''." source="com.puppycrawl.tools.checkstyle.checks.HeaderCheck"/>
<error line="32" column="1" severity="error" message="'{' should be on the previous line." source="com.puppycrawl.tools.checkstyle.checks.blocks.LeftCurlyCheck"/>

Instead of something like

<checkstyle version="3.4"> <file name="C:\Programmation\OpenSource\jbcs\src\com\henri\jbcs\gui\CSPropertyGroup.java"> </file> <file name="C:\Programmation\OpenSource\jbcs\src\com\henri\jbcs\gui\CSPropertyGroup.java"> <error line="33" column="4" severity="warning" message="Missing a Javadoc comment." source="com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocMethodCheck"/> <error line="36" column="4" severity="warning" message="Missing a Javadoc comment." source="com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocMethodCheck"/> <error line="39" column="4" severity="warning" message="Missing a Javadoc comment." source="com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocMethodCheck"/> </file> </file> </checkstyle>

This is because in AuditEvent you have this:

public SeverityLevel getSeverityLevel()
{
    return (mMessage == null)
        ? SeverityLevel.INFO
        : mMessage.getSeverityLevel();
}

So if you have acceptOnMatch="true" on anything else
than info, the accept() method will always return false
in methods like:

public void Checker.fireFileStarted(String aFileName)
{
    final String stripped =

getStrippedFileName(aFileName);
final AuditEvent evt = new AuditEvent(this,
stripped);
if (mFilters.accept(evt)) {
final Iterator it = mListeners.iterator();
while (it.hasNext()) {
final AuditListener listener =
(AuditListener) it.next();
listener.fileStarted(evt);
}
}
}

So you get everything filtered (fireAuditStarted,
fireFileStarted, fireAuditFinished, ...), not only the
errors.

Since we cannot remove the accept() in the
fireSomething methods because it should keep working
for the suppression filter I see one solution:

Have a AuditEventType in AuditEvent. That will be nice.
It will make the ugly (mMessage == null) ?
SeverityLevel.INFO code can disappear.

And SeverityMatchFilter will only do something like:

public boolean accept(AuditEvent aEvent)
{

if(!aEvent.getType().equals(AuditEventType.ERROR)) {
return true;
}
boolean result =
mSeverityLevel.equals(aEvent.getSeverityLevel());
if (mAcceptOnMatch) {
return result;
}
else {
return !result;
}
}

If you agree with this solution, I am willing to
provide a patch.

The test case would be something like this:

public void testNotErrorMessage()
{
   filter.setSeverity("error");
   filter.setAcceptOnMatch(true);
   final AuditEvent ev = new AuditEvent(this,

"Test.java");
assertTrue(filter.accept(ev));
}

Discussion

  • Oliver Burn
    Oliver Burn
    2004-12-07

    Logged In: YES
    user_id=218824

    Personally I think that the filter should only be applied in
    the Checker.fireErrors() method.

    Otherwise you get the bug you have described where invalid
    XML is generated.

    Thoughts?

     
  • Henri Tremblay
    Henri Tremblay
    2004-12-07

    Logged In: YES
    user_id=893525

    From what I saw, the suppression filter was also used to
    competly remove notifications for a file. So if I suppress a
    class without specifying a line or column, the
    fireFileStarted won't be called. This means that for example
    the xml report will be cleaner since there won't be
    <file></file> tags for filtered files...

    Luckily, since SuppressElement returns true if the message
    is null, fireAuditStarted / Finished don't get filtered by
    the suppresion filter.

    So, the quick solution is to keep the accept() only in
    fireErrors as you said. The ouput will be bigger than
    expected for people using the suppression filter but it will
    be valid.

    The other solution is what I propose to give the filter the
    information about which kind of event it is filtering.

    But feel free to pick any solution you like as soon as the
    bug is fixed :-)

     
  • Oliver Burn
    Oliver Burn
    2004-12-07

    Logged In: YES
    user_id=218824

    Fixed with the option I selected.