GitHub, generate website

Github, generate report for WriteTag/all-examples-in-one

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c289e27_2024001956/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c289e27_2024001956/checks/javadoc/writetag.html#Description

Report for WriteTag/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/c289e27897e27b14a1e22aaab72d1ed3cdbcb58d_2024010456/reports/diff/index.html

So resetting the severity level back to the original level line 251 here causes a VoidMethodCallMutator in pitest

To actually solve this though it would be kind of a big change
Currently all violations are verified (actual vs expected) based on their line number and violation message in all the verify methods and verifyViolations in AbstractModuleTestSupport

We could try to add a way for verifying the expected vs actual severity level of violations also but I think this is too big a change for a very rare case like this WriteTag one which could just be suppressed

e.g: I thought that this was testing the severity level but it is actually testing whether it will break or not not the severity level being the same or not at the end of the check since there is no direct way to use the expected vs actual severity level in any of the current verify methods

I am rooting for just suppressing this mutation since this is a very rare case that only happens in this check.
WDYT ? @romani

whole codebase so the easy solution would be to just suppress this mutation

It is always easy to ignore. But whole our project doesn't not ignore quality metrics.
No suppressions, if rare it ok, just need test on it.

https://checkstyle.org/config.html#Severity
Vs
https://checkstyle.org/checks/javadoc/writetag.html#Properties tagSeverity

Do you see a reason why we have this special property in this Check?

Sounds like we can drop such special property as all Checks already have it.

We have 1 logTag( that plays with severity and 2 log that use severity defined in usual way (common for all Checks).

Ideally we should just change default behavior in this PR, and do removal of property in separate issue if we see no reason to have it Check.

log(lineNo, MSG_MISSING_TAG, tag);

We just remove this violation, so we should not be involved in much bigger update

It is always easy to ignore. But whole our project doesn't not ignore quality metrics.
No suppressions, if rare it ok, just need test on it.

I mean it is kind of a big change we would need to add a new verify method because currently there is no way to add a new test that would compare the expected vs actual severity level. We can not just add a new test in the suite for the check because there is no way we can test for the severity level with the way verify and verifyViolations work rn , but a big change to the verification methods to add a way in here or just add a new method like verifyViolations which would be breaking compatability i think ?

https://checkstyle.org/config.html#Severity
Vs
https://checkstyle.org/checks/javadoc/writetag.html#Properties tagSeverity

Do you see a reason why we have this special property in this Check?

Honestly it doesnt seem to bring up any benefit to change the severity level for this check idk why this check needs this property probably better off without it

We have 1 logTag( that plays with severity and 2 log that use severity defined in usual way (common for all Checks).
Ideally we should just change default behavior in this PR, and do removal of property in separate issue if we see no reason to have it Check.

Sure but for this PR's changes to pass we would need to fix the mutation of the changed severity level thing so should we start with the removal of this property first then continue here ? Most likely there is no more change in this PR it handles the default behaviour rn the only thing missing is fixing the mutation which would not be needed if the property is removed altogether

Purpose is to give a different severity between a tag format match versus everything else this check logs.

Purpose is to give a different severity between a tag format match versus everything else this check logs.

Ok makes sense . Ill try to fix its mutation then

there are a lot of complains on this Check - https://github.com/checkstyle/checkstyle/issues?q=is%3Aissue+is%3Aopen+tagSeverity

I pretty sure we will remove this property from this Check in future (breaking compatibility). But let not do this in this PR.

2 issues (now 1 since you closed the other) and 6 closed is not a lot.

This property has a purpose to this check, so if you remove it, you remove the purpose of this check. You might as well remove the check completely then.

If your complaint is with 2 issues, doesn't make sense why we keep Indentation which is a worse check then this with 75 issues https://github.com/checkstyle/checkstyle/issues?q=is%3Aopen+is%3Aissue+label%3Aindentation (67 closed).

@MohanadKh03 , do you need help with mutation ?

After some trial-and-error it seems the only possible way is to add a way to validate the expected vs actual severity levels but currently it is not possible
The mutation is happening because of a Void Method Call Mutator which basically erases the resetting here at 251

We can add a new test that checks that with the default configs there is no change to the severity level before and after processing check to ensure this doesnt happen in other future checks as well we can make it inside XpathRegressionTest for example ?
Yes if you have a better idea please let me know @romani

Ok, our common testing but Inputs with config inside Input will not work here.
It is ok. I confirming exceptional case.

We can do it in low level way as we did long time ago.
We can setup config by means of beans and receive violations from any logger.

Example of how to run checker

This is example how to set listener

And we just need to add above test method a comment that briefly explains reasons of doing testing so low level.

Windows CI problem:

[ERROR]   WriteTagCheckTest.testResetSeverityLevel:218 Second violation's severity level should have been reset back to default (error)
value of: toLowerCase(...)
expected: error
but was : writetag
[INFO] 
[ERROR] Tests run: 5148, Failures: 1, Errors: 0, Skipped: 337

https://ci.appveyor.com/project/Checkstyle/checkstyle/builds/50955311/job/lkjqq6iv4503lech#L1367

Most likely due to different end lines symbols.

@romani
The checkstyle.checkstyle (Job test-ru) seems to have somehow got stuck ? It has been going on for a while after it got cancelled