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 #10924: Checks that use getLine() should check code points for spaces #10976

Closed
wants to merge 1 commit into from

Conversation

MUzairS15
Copy link
Contributor

@MUzairS15 MUzairS15 commented Nov 23, 2021

Closes #10924

@MUzairS15
Copy link
Contributor Author

MUzairS15 commented Nov 23, 2021

I also redefined hasWhitespaceBefore because it was using characters to check for spaces.
I want to understand, pls help.

@MUzairS15
Copy link
Contributor Author

MUzairS15 commented Nov 26, 2021

Hello, @nmancus1 Pitest are failing, on inspecting it seems like new method getCodePoints() added in AbstractCheck,and
document generated for that is creating the problem, Similarly in GenericWhiteSpaceCheck.
Not sure if my understanding is correct, can you take a look?

@MUzairS15
Copy link
Contributor Author

How to generate regression report?
#10924 (comment)

@nrmancuso
Copy link
Member

@MUzairS15 take a look at https://github.com/checkstyle/contribution/tree/master/checkstyle-tester#report-generation, this section outlines report generation process. For an example, see #10838

@MUzairS15
Copy link
Contributor Author

MUzairS15 commented Dec 1, 2021

getLineCodePoints or getCodePointsFromLine?
Will try to solve coverage failure. Any idea on tests to achieve 100% coverage?

@redhairrs
Copy link

Sir, I am new here how can I work on this problem?

@MUzairS15
Copy link
Contributor Author

I am already working on this.

@MUzairS15
Copy link
Contributor Author

Still not able to meet coverage in SingleSpaceSeparator in isFirstInLine method return indexOfNonWhitespace(codePoints) >= columnNo + 1 || codePoints.length == 0 ;
and
isBlockCommentEnd. Partial coverage in both of these.

@romani
Copy link
Member

romani commented Dec 4, 2021

thinks about age case that you can create in Input file to cover them, attentively read coverage report to understand what exactlt is not covered.

@MUzairS15 MUzairS15 force-pushed the iss-10924 branch 2 times, most recently from 82c7cef to f6d8b28 Compare December 17, 2021 07:38
@MUzairS15 MUzairS15 marked this pull request as ready for review December 17, 2021 10:57
@MUzairS15 MUzairS15 force-pushed the iss-10924 branch 2 times, most recently from af80a4f to 218f75f Compare December 17, 2021 16:29
@MUzairS15
Copy link
Contributor Author

I realised, I missed test for overloaded hasWhiteSpaceBefore in CommonUtil, @nmancus1 can you suggest more tests to make sure pitest passes?

@nrmancuso
Copy link
Member

nrmancuso commented Dec 17, 2021

@MUzairS15 please make all other CI happy, then we will take a look at failing pitest. Also, it would be good if you could leave a "review" (comment) on any suppressions that you have added explaining why they are needed.

Edit: do not worry about failure in semaphore

@MUzairS15 MUzairS15 force-pushed the iss-10924 branch 2 times, most recently from 1cd6c6a to 21ba187 Compare December 18, 2021 06:54
@MUzairS15
Copy link
Contributor Author

I need a empty array to be created, to test for subArray, but because of this IDEA inspection is failing. Can someone suggest how to tackle this. final int[] expected3 = new int[0];

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.

Few things to start with:

@@ -364,6 +369,13 @@ public void testIsBlank() {
assertFalse(CommonUtil.isBlank("string"), "Should return false when string is not empty");
}

@Test
public void testIsBlankCodePoint() {
final int[] input = {115, 116, 114, 105, 110, 103};
Copy link
Member

Choose a reason for hiding this comment

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

Please make all code point array initializations in tests like:

      ... =  "my string here".codePoints().toArray();

Tests also serve as documentation; this will make these tests much easier to read (and write).

This should help with #10976 (comment) also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

* Contains String Utility methods.
*
*/
public final class StringUtil {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this CodePointUtil, since all methods deal with int/ int [] type and not String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@MUzairS15 MUzairS15 force-pushed the iss-10924 branch 2 times, most recently from 215805c to 63e2650 Compare December 21, 2021 11:58
@romani
Copy link
Member

romani commented Dec 22, 2021

@MUzairS15 , please do not resolve conversations, only author of comment can resolve them

@nrmancuso
Copy link
Member

@MUzairS15 let's close this PR since we have decided to do separate PRs for #10924 in #10994 and others

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.

All checks that use AbstractCheck#getLine() should check code points instead of characters for spacing
5 participants