Issue #4607: Added moduleId to violation messages #4899

Luolc · 2017-08-04T17:00:18Z

In XMLLogger, there would be an id="..." if the module id is set. (and no source="...")

Log message would be [SEVERITY LEVEL] filePath:lineNo:columnNo: message. [ModuleId], if module id is set.

I was stuck before for I didn't realize that I need to modify compareTo in LocalizedMessage.

romani · 2017-08-04T18:36:54Z

CIs should be green

rnveach

@Luolc We need a test case where 2 violations are printed with the same text and line number for the same file from running 2 instances of the check. 1 instance should have an ID and the other shouldn't. We expect both violations to be printed out.
This caused us a bug in master. See #4607 (comment) .

rnveach · 2017-08-04T22:03:15Z

src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java

@@ -415,7 +416,11 @@ public int compareTo(LocalizedMessage other) {

        if (lineNo == other.lineNo) {
            if (columnNo == other.columnNo) {
-                result = getMessage().compareTo(other.getMessage());
+                result = Comparator.nullsFirst(Comparator.<String>naturalOrder())


@romani

Classes which are missed in Cobertura coverage report:
com/puppycrawl/tools/checkstyle/api/LocalizedMessage

It looks like either Comparator.nullsFirst or Comparator.naturalOrder forces cobertura to ignore the entire class like we have seen with streams.

@Luolc Please rewrite this class not using those 2 methods and figure out which is causing the problem.

@Luolc , please add such classes to known list - https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml#L19 to disallow usage of them till we are on cobertura. Such update could be separate PR, to avoid mixing too much in one PR.

codecov-io · 2017-08-07T08:59:01Z

Codecov Report

Merging #4899 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4899   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         287     287           
  Lines       15488   15501   +13     
  Branches     3518    3523    +5     
======================================
+ Hits        15488   15501   +13

Impacted Files	Coverage Δ
...l/tools/checkstyle/AuditEventDefaultFormatter.java	`100% <100%> (ø)`	⬆️
...pycrawl/tools/checkstyle/api/LocalizedMessage.java	`100% <100%> (ø)`	⬆️
...ava/com/puppycrawl/tools/checkstyle/XMLLogger.java	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14a22ed...0dc6ed7. Read the comment docs.

romani

as CI become green , I am ok to merge.

Luolc · 2017-08-07T10:46:42Z

@romani I just added a UT as rnveach said at #4899 (review)

I cannot run verify currently due to #4842. CI failed and I would have a check after I get home to use my Mac

romani · 2017-08-07T11:11:27Z

I cannot run verify currently due

If problem is only at MainTest.testCreateListenerWithLocationIllegalStateException, you can exclude it execution -Dtest='*,!MainTest' , it will fail on missed coverage, but you can recheck coverage manually.

rnveach · 2017-08-07T12:51:54Z

I cannot run verify currently due to #4842.

@Luolc I get around this issue currently by running verify in pieces similar to like travis does. Look at travis.yml to see the commands separated out. Just remember to delete the file before running Checkstyle on itself.

Luolc · 2017-08-07T13:02:38Z

@romani @rnveach Thanks for the help.

I updated the IDEA inspection config. There would be two more classes in CheckerTest.
UT for #4899 (review) is placed in CheckerTest. I am not very sure if this is the proper place.

rnveach · 2017-08-07T13:28:39Z

config/intellij-idea-inspections.xml

@@ -332,7 +332,7 @@
    </inspection_tool>
    <inspection_tool class="ClassWithOnlyPrivateConstructors" enabled="true" level="WARNING" enabled_by_default="true" />
    <inspection_tool class="ClassWithTooManyDependencies" enabled="true" level="ERROR" enabled_by_default="true">
-        <option name="limit" value="34" />
+        <option name="limit" value="36" />


@romani We just upped this number because we couldn't find a way to suppress it for a specific file, and here we are upping it again.
Should we just turn it off and report problem to IntelliJ?

lets keep it for now, but will be addressed at #4929

rnveach · 2017-08-07T13:32:14Z

src/main/java/com/puppycrawl/tools/checkstyle/XMLLogger.java

+                        + "\"/>");
+            }
+            else {
+                writer.println(" id=\""


Why is this surrounded by an if if the attributes for both are different. We can have both in XML file if we wish.

Since we are completely overriding the source in the CLI output to only show the ID, we should do the exact same in the XML.
Make this attribute name source and not id.

rnveach · 2017-08-07T13:35:14Z

src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java


        if (lineNo == other.lineNo) {
            if (columnNo == other.columnNo) {
-                result = getMessage().compareTo(other.getMessage());
+                if (Objects.equals(moduleId, other.moduleId)) {
+                    result = getMessage().compareTo(other.getMessage());


@romani I don't see us comparing non-id sources of message. (IE: module name)
If 2 different modules print the exact same message on the same line/column, we will have the same issue as before where we had different ids.
Should this be a new issue where we add module name if id is not specified to the compare?

moved to #4930

rnveach · 2017-08-07T13:43:08Z

src/test/java/com/puppycrawl/tools/checkstyle/AuditEventDefaultFormatterTest.java

@@ -104,6 +104,23 @@ public void testFormatModuleNameDoesNotContainCheckSuffix() {
    }

    @Test
+    public void testFormatModuleWithModuleId() {
+        final AuditEvent mock = PowerMockito.mock(AuditEvent.class);


Why is this a mock? Why can't we instantiate AuditEvent and LocalizedMessage?
Mocks should be a last resort when deep code can be done normally.

You were able to get by without a mock in XMLLoggerTest.

I saw every UT in this class used mock. I assumed there could be some reason so I just followed them.
So should I avoid using mock in this new UT?

I don't know why this is, but since ever test is like this, we'll just leave it as is.

It was my mistake to allow such code to appear, mocks should not be used in simple cases like this.
new issue - #4931

rnveach · 2017-08-07T13:46:04Z

src/test/java/com/puppycrawl/tools/checkstyle/CheckerTest.java

+        // BriefUtLogger does not print the module name or id postfix,
+        // so we need to set logger manually
+        final ByteArrayOutputStream out =
+                (ByteArrayOutputStream) Whitebox.getInternalState(this, "stream");


From here until the end, why can't we call verify(Checker checker, String fileName, String... expected)?
Duplicating all this code isn't acceptable.

The super class uses BriefUtLogger. And it only print message like /path/to/file.java message. It don't have a module name or id postfix. We need smth like /path/to/file.java message [ModuleName or ModuleId]
So I change the logger and rewrite the verify logic. The verify code I wrote has some differences from the exist ones.

@Luolc Can't we modify existing code in AbstractModuleTestSupport to supply an optional different Logger, one that prints full original message?

I was thinking that we only need this kind of verify here. If we don't ever need this anywhere else, is it necessary to modify the base class?
I could changing this into AbstractModuleTestSupport, but I preferred just write it in CheckerTest.

@romani I can hook in to most of the verify but there is some obstacles and weird things preventing a full hook because of the custom AuditEventUtFormatter instead of using the real AuditEventDefaultFormatter.

If you agree this should be fixed, lets move it to another issue. We should have to rewrite AuditEventDefaultFormatter so we can pick and choose what is displayed for UTs, instead of a custom implementation which is not really regressing full message displays.

lets discuss this in separate issue .... over customization mean over complication .... all that is to please "test" code ... hmmm, lets discuss in detail what you propose , I am a bit suspicious.

rnveach · 2017-08-07T13:50:32Z

src/test/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessageTest.java

+        final LocalizedMessage message2 = new LocalizedMessage(0,
+                "com.puppycrawl.tools.checkstyle.checks.coding.messages", "empty.statement",
+                EMPTY_OBJECT_ARRAY, null, LocalizedMessage.class, null);
+        final LocalizedMessage message3 = new LocalizedMessage(0,


It's hard to follow what is the difference between these 3.
Rename message3 to message2 and rename current message2 to messageNull.

I'm also hesitant to say we should put the strings into local final variables to make reading slightly easier.

romani · 2017-08-09T05:27:16Z

shippable is relaunched

rnveach · 2017-08-09T11:49:16Z

@romani Even though you already approved, please see my comments pinning you.

romani · 2017-08-10T21:14:43Z

I commented each point.

rnveach · 2017-08-10T21:52:59Z

Thanks.

rnveach reviewed Aug 4, 2017

View reviewed changes

Luolc force-pushed the issue4607 branch from fcf3f2e to 077f611 Compare August 7, 2017 08:37

Luolc mentioned this pull request Aug 7, 2017

Pull #4907: disallow java.util.Comparator in import-control #4907

Merged

romani approved these changes Aug 7, 2017

View reviewed changes

Luolc force-pushed the issue4607 branch from 077f611 to d4f9702 Compare August 7, 2017 10:34

Luolc force-pushed the issue4607 branch 2 times, most recently from 2ea7028 to 546bd36 Compare August 7, 2017 11:00

Luolc force-pushed the issue4607 branch from 546bd36 to 560409f Compare August 7, 2017 11:58

rnveach requested changes Aug 7, 2017

View reviewed changes

Issue checkstyle#4607: Added moduleId to violation messages

0dc6ed7

Luolc force-pushed the issue4607 branch from 560409f to 0dc6ed7 Compare August 7, 2017 14:51

rnveach approved these changes Aug 8, 2017

View reviewed changes

romani merged commit 78de3dc into checkstyle:master Aug 10, 2017

Luolc deleted the issue4607 branch August 11, 2017 03:10

romani mentioned this pull request Apr 24, 2020

LocalizedMessage should use ModuleName in compareTo method #4930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #4607: Added moduleId to violation messages #4899

Issue #4607: Added moduleId to violation messages #4899

Luolc commented Aug 4, 2017

romani commented Aug 4, 2017

rnveach left a comment •

edited

rnveach Aug 4, 2017

romani Aug 5, 2017

codecov-io commented Aug 7, 2017 •

edited

romani left a comment

Luolc commented Aug 7, 2017

romani commented Aug 7, 2017

rnveach commented Aug 7, 2017 •

edited

Luolc commented Aug 7, 2017

rnveach Aug 7, 2017

romani Aug 10, 2017

rnveach Aug 7, 2017

Luolc Aug 7, 2017

rnveach Aug 7, 2017 •

edited

romani Aug 10, 2017

rnveach Aug 7, 2017

Luolc Aug 7, 2017 •

edited

rnveach Aug 7, 2017

romani Aug 10, 2017

rnveach Aug 7, 2017

Luolc Aug 7, 2017

rnveach Aug 7, 2017

Luolc Aug 7, 2017

rnveach Aug 8, 2017 •

edited

romani Aug 10, 2017

rnveach Aug 7, 2017

rnveach Aug 7, 2017 •

edited

Luolc Aug 7, 2017

romani commented Aug 9, 2017

rnveach commented Aug 9, 2017

romani commented Aug 10, 2017

rnveach commented Aug 10, 2017

Issue #4607: Added moduleId to violation messages #4899

Issue #4607: Added moduleId to violation messages #4899

Conversation

Luolc commented Aug 4, 2017

romani commented Aug 4, 2017

rnveach left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 7, 2017 • edited

Codecov Report

romani left a comment

Choose a reason for hiding this comment

Luolc commented Aug 7, 2017

romani commented Aug 7, 2017

rnveach commented Aug 7, 2017 • edited

Luolc commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Aug 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Aug 9, 2017

rnveach commented Aug 9, 2017

romani commented Aug 10, 2017

rnveach commented Aug 10, 2017

rnveach left a comment •

edited

codecov-io commented Aug 7, 2017 •

edited

rnveach commented Aug 7, 2017 •

edited

rnveach Aug 7, 2017 •

edited

Luolc Aug 7, 2017 •

edited

rnveach Aug 8, 2017 •

edited

rnveach Aug 7, 2017 •

edited