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

TranslationCheckTest.testLogOutput is failed #5141

Closed
romani opened this Issue Sep 23, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Sep 23, 2017

on my pc, build is failing:

[ERROR] Failures: 
[ERROR]   TranslationCheckTest.testLogOutput:190 Unexpected log output expected:<...ationCheckFireErrors[.properties">
</file>
<file name="..../checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/translation/InputTranslationCheckFireErrors_de.properties">
<error line="0" severity="error" message="Key &apos;anotherKey&apos; missing." source="com.puppycrawl.tools.checkstyle.checks.TranslationCheck"/]>
</file>
</checksty...> but was:<...ationCheckFireErrors[_de.properties">
<error line="0" severity="error" message="Key &apos;anotherKey&apos; missing." source="com.puppycrawl.tools.checkstyle.checks.TranslationCheck"/>
</file>
<file name="..../checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/translation/InputTranslationCheckFireErrors.properties"]>
</file>
</checksty...>
[INFO] 
[ERROR] Tests run: 2501, Failures: 1, Errors: 0, Skipped: 0

My linux generate output same as at OutputTranslationCheckMacOS.xml.
image

@romani romani added the approved label Sep 23, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Oct 4, 2017

Member

@romani I am working on this.
I am going to make a new verify routine to compare the XML structure (unordered). This will also ensure that our XML outputs are valid XML files as we skip verifying this in the hard-coded strings.

I will also be replacing the string expected results in XMLLogger with this new verify.

Member

rnveach commented Oct 4, 2017

@romani I am working on this.
I am going to make a new verify routine to compare the XML structure (unordered). This will also ensure that our XML outputs are valid XML files as we skip verifying this in the hard-coded strings.

I will also be replacing the string expected results in XMLLogger with this new verify.

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 4, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Oct 4, 2017

romani added a commit that referenced this issue Oct 7, 2017

romani added a commit that referenced this issue Oct 7, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Dec 15, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Dec 15, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 16, 2017

Member

@rnveach ,

Looks like I did bad job by reporting issue that is not issue any more.
Look like we need to open new issue with clear definition of what is wrong, what block us, and what we want to improve and to let us archive smth.

This will also ensure that our XML outputs are valid XML files as we skip verifying this in the hard-coded strings.

This should be a routine of XmlLogger. We need to have a schema for it.
Might be better to use - #5166 .

===============
Several questions:

Why XML is only affected ? Check should report the same to any Logger. Strange that we focused on XMLLogger here when we try to test Check.

Why TranslationCheck care about XML report that much ? (non of other Checks are affected)

Why order is not deterministic for files?

Member

romani commented Dec 16, 2017

@rnveach ,

Looks like I did bad job by reporting issue that is not issue any more.
Look like we need to open new issue with clear definition of what is wrong, what block us, and what we want to improve and to let us archive smth.

This will also ensure that our XML outputs are valid XML files as we skip verifying this in the hard-coded strings.

This should be a routine of XmlLogger. We need to have a schema for it.
Might be better to use - #5166 .

===============
Several questions:

Why XML is only affected ? Check should report the same to any Logger. Strange that we focused on XMLLogger here when we try to test Check.

Why TranslationCheck care about XML report that much ? (non of other Checks are affected)

Why order is not deterministic for files?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 16, 2017

Member

@rnveach ,

Why order is not deterministic for files?

DefaultLogger ( to std output) does not collect/buffer any file related information, just simply redirect fired events to output.
XmlLogger do gathering/caching of events - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/XMLLogger.java#L72.

    private final Map<String, FileMessages> fileMessages =
new ConcurrentHashMap<>();

order is nit deterministic as it is based on hash.
Order of violations in file is stored in List collection (ordered), but Checks order in Checker is not ordered so violations from certain Check will be in order (as AST is ordered), but groups of violations (grouped by Check) will not be in order.
So result output will be not ordered.
Example:

Checker
  +---Check1
         +----- violation1
         +----- violation2
  +---Check2
         +----- violation1
         +----- violation2

output could be:

Check1
     +----- violation1
     +----- violation2
Check2
     +----- violation1
     +----- violation2

or

Check2
    +----- violation1
    +----- violation2
Check1
   +----- violation1
   +----- violation2

BUT this is not a problem when we have only one Check.

Nuance is in order of files that we from by OS, so lets investigate is it ordered or not....
Checker(RootModule) receive ordered collection of files to process
int process(List<File> files) throws CheckstyleException; link.

So, I see non-deterministic order only in XMLLogger.java, due to its caching of files that come to it in hashed collection.
So if problem in XMLLogger , why not to fix root of problem ?

Member

romani commented Dec 16, 2017

@rnveach ,

Why order is not deterministic for files?

DefaultLogger ( to std output) does not collect/buffer any file related information, just simply redirect fired events to output.
XmlLogger do gathering/caching of events - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/XMLLogger.java#L72.

    private final Map<String, FileMessages> fileMessages =
new ConcurrentHashMap<>();

order is nit deterministic as it is based on hash.
Order of violations in file is stored in List collection (ordered), but Checks order in Checker is not ordered so violations from certain Check will be in order (as AST is ordered), but groups of violations (grouped by Check) will not be in order.
So result output will be not ordered.
Example:

Checker
  +---Check1
         +----- violation1
         +----- violation2
  +---Check2
         +----- violation1
         +----- violation2

output could be:

Check1
     +----- violation1
     +----- violation2
Check2
     +----- violation1
     +----- violation2

or

Check2
    +----- violation1
    +----- violation2
Check1
   +----- violation1
   +----- violation2

BUT this is not a problem when we have only one Check.

Nuance is in order of files that we from by OS, so lets investigate is it ordered or not....
Checker(RootModule) receive ordered collection of files to process
int process(List<File> files) throws CheckstyleException; link.

So, I see non-deterministic order only in XMLLogger.java, due to its caching of files that come to it in hashed collection.
So if problem in XMLLogger , why not to fix root of problem ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 16, 2017

Member

Checker(RootModule) receive ordered collection of files to process
int process(List files) throws CheckstyleException;

You mean a collection of files. We do not control order from CLI as we use listFiles (no ordered guaranteed).
We do control our order of files in our UT, as everything is Arrays and ArrayLists.

So if problem in XMLLogger , why not to fix root of problem ?

We have stated before we do not care about order. Order of modules executing, order of files, etc...
Why do we want to care here?

Member

rnveach commented Dec 16, 2017

Checker(RootModule) receive ordered collection of files to process
int process(List files) throws CheckstyleException;

You mean a collection of files. We do not control order from CLI as we use listFiles (no ordered guaranteed).
We do control our order of files in our UT, as everything is Arrays and ArrayLists.

So if problem in XMLLogger , why not to fix root of problem ?

We have stated before we do not care about order. Order of modules executing, order of files, etc...
Why do we want to care here?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 16, 2017

Member

Why do we want to care here?

We do not sorting anything, as use them as come to us. So user control this to some extent.

I kind of try to find clear reason , why we become affected only now and why I need this code in our repo.

Member

romani commented Dec 16, 2017

Why do we want to care here?

We do not sorting anything, as use them as come to us. So user control this to some extent.

I kind of try to find clear reason , why we become affected only now and why I need this code in our repo.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 17, 2017

Member

why I need this code in our repo.

This test kills this mutation http://rveach.no-ip.org/checkstyle/regression/pitest-reports/13/com.puppycrawl.tools.checkstyle.checks/TranslationCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@c2c937c_467 .
It doesn't kill the other one, haven't looked into this.

so you want to fix XMLLogger in this issue (if it is possible)?

Member

rnveach commented Dec 17, 2017

why I need this code in our repo.

This test kills this mutation http://rveach.no-ip.org/checkstyle/regression/pitest-reports/13/com.puppycrawl.tools.checkstyle.checks/TranslationCheck.java.html#org.pitest.mutationtest.report.html.SourceFile@c2c937c_467 .
It doesn't kill the other one, haven't looked into this.

so you want to fix XMLLogger in this issue (if it is possible)?

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

romani added a commit that referenced this issue Dec 25, 2017

@romani romani added this to the 8.6 milestone Dec 25, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 25, 2017

Member

so you want to fix XMLLogger in this issue (if it is possible)?

ok, lets merge this issue, one day it will come back, and probably we will have better view of how to fix this.
XML report validation that is not based on order is ok to have, I hope we will not misuse it.

Member

romani commented Dec 25, 2017

so you want to fix XMLLogger in this issue (if it is possible)?

ok, lets merge this issue, one day it will come back, and probably we will have better view of how to fix this.
XML report validation that is not based on order is ok to have, I hope we will not misuse it.

@romani romani closed this Dec 25, 2017

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