Skip to content

Issue #15: provide UT for correct patch file loading that contains multiple files in it #16

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
Merged
merged 1 commit into from
Jun 20, 2020
Merged

Issue #15: provide UT for correct patch file loading that contains multiple files in it #16

merged 1 commit into from
Jun 20, 2020

Conversation

HuGanghui
Copy link
Contributor

@HuGanghui HuGanghui commented Jun 14, 2020 •
edited
Loading

Issue #15: provide UT for correct patch file loading that contains multiple files in it

I add all cases in comments to UT, and now UT just check changed lines statistics.
And because fields are private so I use reflection.

@baratali baratali linked an issue Jun 14, 2020 that may be closed by this pull request
provide UT for correct patch file loading that contains multiple files in it #15
Closed
@baratali baratali requested review from baratali and removed request for baratali June 14, 2020 13:10
@romani
Copy link
Member

romani commented Jun 14, 2020

@HuGanghui , please explain what problem in patch loading we have , that you raised at java-diff-utils/java-diff-utils#82 . Please create issue on them in our issue tracker. If no problem ... please close issue in library and post solution to close comment.

@HuGanghui
Copy link
Contributor Author

@romani no problem on java-diff-utils/java-diff-utils#82, I have closed it.

@romani
Copy link
Member

romani commented Jun 16, 2020

@HuGanghui , please remove from this PR updates on SuppressionPatchFilterElement.java

this pure patch load test(different styles), no relationship to Checkstyle at all.

if you need upodate in Filter , please make separate issue on this and it will come after this PR.
Please finish this PR ASAP, with hight priority then anyting alse.

@HuGanghui
Copy link
Contributor Author

@romani done, I have removed from this PR updates on SuppressionPatchFilterElement.java

@romani
Copy link
Member

romani commented Jun 17, 2020

I do not understand why this test needy filter.
We test abilities of library, not a filter.
Please remove from this test all reference of checkstyle and just use library and input files.

@HuGanghui
Copy link
Contributor Author

HuGanghui commented Jun 17, 2020 •
edited
Loading

@romani, because there is a issue java-diff-utils/java-diff-utils#83 in java-diff-utils when one patch file have multiple files , so I deal with this situation by ourself in

patch-filters/src/main/java/com/github/checkstyle/patchfilter/SuppressionPatchFilter.java

Lines 99 to 165 in 7898201

private PatchFilterSet loadPatchFile(String patchFileName) throws IOException {
final List<String> originPatch = Files.readAllLines(new File(patchFileName).toPath());
final List<List<String>> patchList = getPatchList(originPatch);
for (List<String> singlePatch : patchList) {
final String fileName = getFileName(singlePatch);
final Patch<String> patch = UnifiedDiffUtils.parseUnifiedDiff(singlePatch);
final List<List<Integer>> lineRangeList = getLineRange(patch);
final SuppressionPatchFilterElement element =
new SuppressionPatchFilterElement(fileName, lineRangeList);
filters.addFilter(element);
}
return filters;
}
/**
* To seperate different files's diff contents when there are multiple files in patch file.
* @param originPatch List of String
* @return patchedList List of List of String
*/
private List<List<String>> getPatchList(List<String> originPatch) {
final List<List<String>> patchedList = new ArrayList<>();
boolean flag = true;
List<String> singlePatched = new ArrayList<>();
for (String str : originPatch) {
if (str.startsWith("diff ")) {
if (flag) {
flag = false;
}
else {
patchedList.add(singlePatched);
singlePatched = new ArrayList<>();
}
}
singlePatched.add(str);
}
patchedList.add(singlePatched);
return patchedList;
}
private String getFileName(List<String> singlePatch) {
String fileName = null;
for (String str : singlePatch) {
if (str.startsWith("+++ ")) {
fileName = str.split("\\s")[1];
final String fileSplit = "/";
if (fileName.contains(fileSplit)) {
final String[] fileNameArray = fileName.split(fileSplit);
fileName = fileNameArray[fileNameArray.length - 1];
}
break;
}
}
return fileName;
}
private List<List<Integer>> getLineRange(Patch<String> patch) {
final List<List<Integer>> lineRangeList = new ArrayList<>();
for (int i = 0; i < patch.getDeltas().size(); i++) {
final Chunk targetChunk = patch.getDeltas().get(i).getTarget();
final List<Integer> lineRange = new ArrayList<>();
lineRange.add(targetChunk.getPosition());
lineRange.add(targetChunk.getPosition() + targetChunk.getLines().size());
lineRangeList.add(lineRange);
}
return lineRangeList;
}
}

so in test I need filter.

@rdiachenko
Copy link
Contributor

rdiachenko commented Jun 17, 2020 •
edited
Loading

because there is a issue java-diff-utils/java-diff-utils#83 in java-diff-utils when one patch file have multiple files , so I deal with this situation by ourself

@HuGanghui we don't know yet whether it's a bug or a specific behavior. If you meet any problem while testing that library please create an issue on their side AND mark your test which fails with @Ignore annotation (e.g.: @Ignore("https://github.com/java-diff-utils/java-diff-utils/issues/83")). In this way we will have a clear picture of how the library loads multiple file patches and which current problems it has.

So, please remove all checkstyle references and use library only as @romani mentioned.

@HuGanghui
Copy link
Contributor Author

@rdiachenko thanks, I will do it

HuGanghui
HuGanghui commented Jun 17, 2020
View reviewed changes
src/test/java/com/github/checkstyle/patchfilter/SuppressionPatchFilterTest.java Outdated
Comment on lines 144 to 150
@Ignore("error: com.github.difflib.unifieddiff.UnifiedDiffParserException: "
+ "expected file start line not found")
public void testGitFormatPatchLinuxTwo() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a error when test GitFormatPatchLinuxTwo file

src/test/java/com/github/checkstyle/patchfilter/SuppressionPatchFilterTest.java Outdated
Comment on lines 207 to 217
@Ignore("filename parsed error, Expected :<tests/test-check-pyflakes.t>, "
+ "Actual :<tests/test-check-pyflakes.t\tTue Jun 09 17:13:26 2020 -0400>\n")
public void testHgDiffPatch() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a filename parsed error when test HgDiffPatch file

src/test/java/com/github/checkstyle/patchfilter/SuppressionPatchFilterTest.java Outdated
Comment on lines 73 to 74
@Ignore("https://github.com/java-diff-utils/java-diff-utils/issues/83")
public void testMultiChangedFilesOnOnePatchOne() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a issue when test problem_diff_issue51.diff

src/test/java/com/github/checkstyle/patchfilter/SuppressionPatchFilterTest.java Outdated
Comment on lines 80 to 82
assertEquals("diff -U0 old/f1 new/f1", file1.getDiffCommand());
// here is f1 not old/f1 and new/f1
assertEquals("f1", file1.getFromFile());
assertEquals("f1", file1.getToFile());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to parse convention:
https://github.com/java-diff-utils/java-diff-utils/blob/a8e4ce2eff297c7700a6215bacb548e30151b948/java-diff-utils/src/main/java/com/github/difflib/unifieddiff/UnifiedDiffReader.java#L124-L130
there should be old/f1 and new/f1

src/test/java/com/github/checkstyle/patchfilter/SuppressionPatchFilterTest.java Outdated
Comment on lines 86 to 89
assertEquals("diff -U0 old/f2 new/f2", file2.getDiffCommand());
// same issue here is f2 not old/f2 and new/f2
assertEquals("f2", file2.getFromFile());
assertEquals("f2", file2.getToFile());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.
Please put old/f2 and new/f2 in assetEquals. Once issue-83 is fixed in the library, this test should successfully pass.

baratali
baratali suggested changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@baratali baratali left a comment

Choose a reason for hiding this comment

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

Since all these tests do not test a patch filter, I think they should not be located in SuppressionPatchFilterTest and should be moved to a new class (for example, DiffFormatsTest).
@romani , do you agree?

@HuGanghui HuGanghui force-pushed the UT-multi-files branch 3 times, most recently from c4c7ee7 to 6d0355a Compare June 18, 2020 09:55
@romani
Copy link
Member

romani commented Jun 19, 2020

yes, Tests should be out Filter test, there should be no reference to Filters/check and checkstyle at all in this test.

romani
romani requested changes Jun 19, 2020
View reviewed changes
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:

baratali
baratali suggested changes Jun 19, 2020
View reviewed changes
src/test/java/com/github/checkstyle/patchfilter/SuppressionPatchFilterTest.java Outdated
Comment on lines 86 to 89
assertEquals("diff -U0 old/f2 new/f2", file2.getDiffCommand());
// same issue here is f2 not old/f2 and new/f2
assertEquals("f2", file2.getFromFile());
assertEquals("f2", file2.getToFile());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.
Please put old/f2 and new/f2 in assetEquals. Once issue-83 is fixed in the library, this test should successfully pass.

@HuGanghui HuGanghui force-pushed the UT-multi-files branch 2 times, most recently from a8dd3cd to 418e55f Compare June 19, 2020 14:33
romani
romani requested changes Jun 20, 2020
View reviewed changes
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 to improve:

@romani romani merged commit 3d14d15 into checkstyle:master Jun 20, 2020
@HuGanghui HuGanghui deleted the UT-multi-files branch June 21, 2020 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romani romani romani approved these changes

@baratali baratali baratali approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

provide UT for correct patch file loading that contains multiple files in it
4 participants
@HuGanghui @romani @rdiachenko @baratali