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.

Sign up for GitHub

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

Jump to bottom

Issue #7799: Resolve Pitest Issues - AvoidStarImportCheck (1) #7843

Merged
merged 1 commit into from Mar 16, 2020
Merged

Issue #7799: Resolve Pitest Issues - AvoidStarImportCheck (1) #7843

merged 1 commit into from Mar 16, 2020

Conversation

wilcoln
Copy link
Contributor

@wilcoln wilcoln commented Mar 13, 2020 •
edited

Resolve #7799
Please see comments in #7799 for all the details.

@wilcoln wilcoln mentioned this pull request Mar 13, 2020
Closed
@wilcoln wilcoln force-pushed the fix-for-7799 branch 2 times, most recently from be85b6b to faf5fe5 Compare March 13, 2020 21:39
@strkkk strkkk requested a review from romani March 14, 2020 06:15
rnveach
rnveach requested changes Mar 14, 2020
View reviewed changes
Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

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

Regression diff report

Scope: All the projects.
Link: https://wilcoln.github.io/checkstyle-reports/7799/mutation/diff/index.html
Bottom line: No difference found.

Regression is not good enough. Only default module was used in run.
https://checkstyle.org/config_imports.html#AvoidStarImport
This module has many properties, and regression has to use them all and all their possible values.
See https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression

@rnveach rnveach assigned rnveach and unassigned romani Mar 14, 2020
@wilcoln wilcoln requested a review from rnveach March 15, 2020 14:39
@rnveach
Copy link
Member

rnveach commented Mar 15, 2020 •
edited

@wilcoln Was there an issue running regression with 1 config instead of splitting configs into 4?

You will also need to rebase on the latest master.

@rnveach
Copy link
Member

rnveach commented Mar 15, 2020

https://wilcoln.github.io/checkstyle-reports/7799/mutation/conf3/diff/android-launcher/index.html

I get a 404 error and can not confirm the config.

@wilcoln
Copy link
Contributor Author

wilcoln commented Mar 15, 2020

https://wilcoln.github.io/checkstyle-reports/7799/mutation/conf3/diff/android-launcher/index.html

I get a 404 error and can not confirm the config.

my bad, correcting this right now

@wilcoln
Copy link
Contributor Author

wilcoln commented Mar 15, 2020

@wilcoln Was there an issue running regression with 1 config instead of splitting configs into 4?

You will also need to rebase on the latest master.

Not at all, I just thought you wanted me to run regression with different configurations.

@rnveach
Copy link
Member

rnveach commented Mar 15, 2020

I just thought you wanted me to run regression with different configurations.

https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression
For pitest you can have all configs in 1. Giving each one a unique id will tell you who caused the violation when a difference is found.

@wilcoln
Copy link
Contributor Author

wilcoln commented Mar 15, 2020

I just thought you wanted me to run regression with different configurations.

https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#checkstyle-pitest-regression
For pitest you can have all configs in 1. Giving each one a unique id will tell you who caused the violation when a difference is found.

Oh, how convenient! It's a shame I didn't pay more attention to that.

@wilcoln
Copy link
Contributor Author

wilcoln commented Mar 15, 2020

https://wilcoln.github.io/checkstyle-reports/7799/mutation/conf3/diff/android-launcher/index.html

I get a 404 error and can not confirm the config.

It's corrected now.

@wilcoln wilcoln requested a review from pbludov March 15, 2020 16:30
@wilcoln wilcoln force-pushed the fix-for-7799 branch 2 times, most recently from 4304bc2 to d66b9b0 Compare March 15, 2020 19:07
@wilcoln wilcoln force-pushed the fix-for-7799 branch 2 times, most recently from 04d711d to 5608481 Compare March 15, 2020 20:33
@rnveach
Copy link
Member

rnveach commented Mar 15, 2020

If CI passes, can be merged.

@rnveach rnveach merged commit 960d437 into checkstyle:master Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbludov pbludov pbludov approved these changes

@rnveach rnveach rnveach approved these changes

@strkkk strkkk strkkk approved these changes

@romani romani Awaiting requested review from romani

Assignees

@rnveach rnveach

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resolve Pitest Issues - AvoidStarImportCheck (1)
5 participants
@wilcoln @rnveach @pbludov @strkkk @romani