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 #74: Create unit tests for line strategy for Checker checks (non-TreeWalker) #84

Closed
wants to merge 1 commit into from

Conversation

HuGanghui
Copy link
Contributor

Issue #74: Create unit tests for line strategy for Checker checks (non-TreeWalker)

@HuGanghui HuGanghui force-pushed the UT-for-test branch 2 times, most recently from 6e4d366 to 3906ebf Compare July 15, 2020 03:56
@HuGanghui HuGanghui marked this pull request as draft July 15, 2020 06:43
@HuGanghui HuGanghui marked this pull request as ready for review July 15, 2020 08:48
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

items (please apply comments to all files):

@@ -0,0 +1,31 @@
diff --git a/src/main/java/Checker/FileLength/Test.java b/src/main/java/Checker/FileLength/Test.java
Copy link
Member

Choose a reason for hiding this comment

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

please fix path to files, it might not be critical in this particular case, but will be good practice ones we come to multifile and different folders diffs.

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

diff --git a/src/main/java/Checker/FileLength/Test.java b/src/main/java/Checker/FileLength/Test.java
index f8603ef..d10d86a 100644
--- a/Test/Test.java
+++ b/Test/Test.java
Copy link
Member

Choose a reason for hiding this comment

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

please fix path here too.

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

@rdiachenko
Copy link
Contributor

Merged in scope of #86

@rdiachenko rdiachenko closed this Jul 15, 2020
public class Test {
// Long line ----------------------------------------------------------------
// Contains a tab -> <- //warn
// Contains trailing whitespace ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you don't need trailing spaces in this input file. FileTabCharacter doesn't check them

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

@@ -0,0 +1,14 @@
package Checker.FileLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments to all input files where you expect a violation. It's hard to always compare with expected.txt and patch.diff.
Something like: // violation newline/patchedline or // violation newline (depends on violation and strategy)

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

@@ -0,0 +1,4 @@
Starting audit...
[ERROR] /Users/hgh/Downloads/my_file/IDEA_workspace/patchfiltergeneratepatch/src/main/java/Checker/RegexpMultiline/Test.java:18: Line matches the illegal pattern 'System\.out\.\n print\('. [RegexpMultiline]
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is not expected as I understand, because patch changes line 19, and this violation is for number 18. This should be covered by context strategy. Am I correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baratali yes, you are correct, and I have add notes in UT about RegexpMultiline that this situation will be solved in context strategy.

Copy link
Contributor

Choose a reason for hiding this comment

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

@HuGanghui , please remove this line if it's not expected in patchedline strategy.

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

4 participants