Skip to content
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

Issue #12430: Resolve Pitest suppression for SeparatorWrapCheck #12432

Merged
merged 1 commit into from Nov 22, 2022

Conversation

arinmodi
Copy link
Contributor

@arinmodi arinmodi commented Nov 19, 2022

@nrmancuso
Copy link
Member

nrmancuso commented Nov 19, 2022

@arinmodi please generate check regression report by github action as specified at #12341, let's see if we can find some failure in SeparatorWrapCheck from your change.

@nrmancuso nrmancuso self-requested a review November 19, 2022 17:01
@nrmancuso nrmancuso self-assigned this Nov 19, 2022
@arinmodi
Copy link
Contributor Author

@nrmancuso , I am confused which file uri i have to pass for regression configs

@nrmancuso
Copy link
Member

@nrmancuso , I am confused which file uri i have to pass for regression configs

See example PRs in parent issue, you need to create a config in a gist and share raw link

@arinmodi
Copy link
Contributor Author

Github, generate report

@arinmodi
Copy link
Contributor Author

Did i correctly generate report ?

@arinmodi
Copy link
Contributor Author

There is no exception or differences in report, so Do i have to find a new test case ?

@nrmancuso
Copy link
Member

There is no exception or differences in report, so Do i have to find a new test case ?

@arinmodi please analyze code and try to determine if we can find a test case, and share analysis here

@arinmodi
Copy link
Contributor Author

Github, generate report

@arinmodi
Copy link
Contributor Author

@nrmancuso, violations found in above report

@nrmancuso
Copy link
Member

@nrmancuso, violations found in above report

Good, please restore code, and create new test case/ test input based on your findings and remove suppression.

@arinmodi
Copy link
Contributor Author

Little confusion,

image

There is no difference in checkstyle link, is difference in other links considered or just differences of checkstyle link is considered ?

@nrmancuso
Copy link
Member

@arinmodi the idea of the report is to find differences in check behavior on a bunch of projects; where we find the differences doesn't matter, we just need to add such code to our tests.

@arinmodi
Copy link
Contributor Author

@arinmodi the idea of the report is to find differences in check behavior on a bunch of projects; where we find the differences doesn't matter, we just need to add such code to our tests.

Got it, That's what I think also, but it's better to ask

@arinmodi
Copy link
Contributor Author

arinmodi commented Nov 20, 2022

image

where can I find this file given as path ?

@nrmancuso
Copy link
Member

@arinmodi see link on the left of the row to view source file, also see projects file at https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/projects-to-test-on-for-github-action.properties to help you to locate these projects online or clone locally

@arinmodi
Copy link
Contributor Author

@nrmancuso, Test File Added

@nrmancuso
Copy link
Member

@arinmodi mutation is still surviving, please investigate . Make sure that config that produced violation in regression report is identical to the one in your new input file and that input file code matches.

@arinmodi
Copy link
Contributor Author

image

I am getting these error when building the project

@nrmancuso
Copy link
Member

nrmancuso commented Nov 21, 2022

image

I am getting these error when building the project

@arinmodi when asking for help, it is best to follow a pattern similar to the bug report template using the console:

  1. Show the command you are running
  2. Show the output of the command
  3. Share what you are expecting
  4. Share what is actually happening

Example:
I am expecting to build and verify checkstyle, but I get the following errors instead and project fails to build:

➜  ~ mvn clean verify
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.045 s
[INFO] Finished at: 2022-11-21T08:11:38-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] The goal you specified requires a project to execute but there is no POM in this directory (/home/nick). Please verify you invoked Maven from the correct directory. -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MissingProjectException
➜  ~ 


This allows others to hopefully reproduce your problem; screenshots are rarely helpful except in the case of website content or some other UI focused issue.

@arinmodi
Copy link
Contributor Author

arinmodi commented Nov 21, 2022

I am trying to run testCommaOnNewLine test, but I got the following error and checkstyle build failed with following error

Executing pre-compile tasks...
Running 'before' tasks
Checking sources
Copying resources... [checkstyle]
Parsing java… [checkstyle]
Checking dependencies… [checkstyle]
Dependency analysis found 0 affected files
Errors occurred while compiling module 'checkstyle'
javac 11.0.13 was used to compile java sources
Finished, saving caches…
Compilation failed: errors: 100; warnings: 4
Executing post-compile tasks...
Compilation failed: errors: 100; warnings: 4
Synchronizing output directories...
21-11-2022 19:33 - Build completed with 100 errors and 4 warnings in 5 sec, 807 ms
D:\Open-Source-Projects\checkstyle\src\main\java\com\puppycrawl\tools\checkstyle\api\JavadocTokenTypes.java:22:55
java: package com.puppycrawl.tools.checkstyle.grammar.javadoc does not exist

@nrmancuso
Copy link
Member

@arinmodi it looks like you have cleaned but not generated antlr classes. Run mvn clean verify first, always run this first if you run into any compilation problems.

@arinmodi
Copy link
Contributor Author

I am trying to analyze the pitest report but got following error.

Command :

groovy .ci/pitest-survival-check-xml.groovy "pitest-whitespace"

Error :

org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
D:\Open-Source-Projects\checkstyle\.ci\pitest-survival-check-xml.groovy: 5: unable to resolve class groovy.util.slurpersupport.GPathResult
 @ line 5, column 1.
   import groovy.util.slurpersupport.GPathResult
   ^

D:\Open-Source-Projects\checkstyle\.ci\pitest-survival-check-xml.groovy: 6: unable to resolve class groovy.util.slurpersupport.NodeChildren
 @ line 6, column 1.
   import groovy.util.slurpersupport.NodeChildren
   ^

2 errors

@nrmancuso
Copy link
Member

@arinmodi make sure that you are using the same version of groovy that we use in CI

@arinmodi
Copy link
Contributor Author

@arinmodi make sure that you are using the same version of groovy that we use in CI

which version ci using ?

@nrmancuso
Copy link
Member

@arinmodi examine pitest execution in CI, all details of installation are there

@arinmodi arinmodi force-pushed the PitestSuppression branch 2 times, most recently from 53ea197 to ebc6569 Compare November 21, 2022 19:51
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Items:

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more item:

config/checkstyle_resources_suppressions.xml Outdated Show resolved Hide resolved
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to merge

@nrmancuso nrmancuso assigned rnveach and unassigned nrmancuso Nov 22, 2022
@nrmancuso
Copy link
Member

@arinmodi please look through other pitest suppression files for trim usage, see if we can find similar cases

@rnveach
Copy link
Member

rnveach commented Nov 22, 2022

Not merging yet as I see no CI runs in my view.

@nrmancuso
Copy link
Member

@arinmodi please rebase on latest master, let's see if we can get CI running again

@arinmodi
Copy link
Contributor Author

@arinmodi please rebase on latest master, let's see if we can get CI running again

Done

@arinmodi
Copy link
Contributor Author

arinmodi commented Nov 22, 2022

By the way, In PR #12317 Kevin is not generating any regression report and still able to find the errors, how he is doing that ? I want to try the same

@rnveach
Copy link
Member

rnveach commented Nov 22, 2022

He has dealt with other PRs with similar changes. Also looking at the code, sometimes it can just be seen on how to create a case to kill the mutation. Regression is needed if your unsure or production code needs to be changed to resolve the mutation.

@arinmodi
Copy link
Contributor Author

He has dealt with other PRs with similar changes. Also looking at the code, sometimes it can just be seen on how to create a case to kill the mutation. Regression is needed if your unsure or production code needs to be changed to resolve the mutation.

so it requires experience ?

@nrmancuso
Copy link
Member

nrmancuso commented Nov 22, 2022

@arinmodi it requires only careful analysis of code and consideration of all possible inputs; experience is helpful but not required :)

@rnveach rnveach merged commit 55e09e5 into checkstyle:master Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants