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

test(smpl): migrate SMPL tests to JUnit5 #4598

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

MartinWitt
Copy link
Collaborator

@MartinWitt MartinWitt commented Feb 12, 2022

Let me try to reduce the diff first, before anyone reviews. And fix my git usage....

Edit 1: Git usage fixed.

@algomaster99
Copy link
Contributor

@MartinWitt don't force push the reduced diff. I would want to analyse what are the code elements that can be removed to minimise diff.


/**
* Note: the patch used in this test has been significantly modified from the C4J original.
*/
public class C4JSetTextSizeTest {
private static ZippedCodeBaseTestContext ctx = null;

/*@BeforeClass
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was this comment important?

Copy link
Contributor

@algomaster99 algomaster99 Feb 12, 2022

Choose a reason for hiding this comment

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

I don't think so. But there was a change like this:

-(method) ->
+method ->

Parenthesis were removed.

Basically, I look for changes which are purely formatting and doesn't add any functional change to the code. GitHub identifies whitespace as one and they have an option to remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a result of the new DJPP with lambda fix #4448

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, do you have any suggestions how I could go about collecting such PRs?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a result of the new DJPP with lambda fix #4448

Right. I am trying to add an option to GitHub which removes such changes as well. We feel that minimising such changes will help aid the review process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR with large diffs which could be reduced? I would mine huge git repos from scientific non industry grade software, because there should huge time pressure and code quality is often not a goal there. Larger merges are probably more typically filled with these noises. Another good candidate could be students developing software having their first contact with git. There could be data, a problem, but at KIT I believe we have for starters a programming course where some use git horribly. Other universities could have this data too. Maybe you could ask them? If you need help with mining GitHub, feel free to ask or mention me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I am trying to add an option to GitHub which removes such changes as well. We feel that minimising such changes will help aid the review process.

I strongly believe creating a list of add/deleted/modified AST elements should be possible with spoon.
Then you map from git diffs to AST elements using their position(fair warning here, I'm unsure if this is possible). Then you should have list of changed AST elements, which are either semantic or formatting changed. To detect the formatting changes, you could compare their toString() results. Using the DJPP for printing should produce deterministic results.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I strongly believe creating a list of add/deleted/modified AST elements should be possible with spoon.

Indeed it is :) https://github.com/spoonlabs/gumtree-spoon-ast-diff

That being possible was the basis of my master's thesis where I made a merge tool on top of Spoon, see https://github.com/kth/spork

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a way to get the AST diffs like Simon said.

Then you map from git diffs to AST elements using their position(fair warning here, I'm unsure if this is possible).

That's a really great idea. Thanks! Getting AST element from source position is something what Sorald also does. See BestFitScanner.

Thanks for the idea about mining GitHub repositories. I will look for datatsets which focus on "bad code quality".

@MartinWitt
Copy link
Collaborator Author

@MartinWitt don't force push the reduced diff. I would want to analyse what are the code elements that can be removed to minimise diff.

Sure go ahead.

@MartinWitt MartinWitt changed the title wip: test(smpl): migrate SMPL tests to JUnit5 test(smpl): migrate SMPL tests to JUnit5 Feb 13, 2022
@MartinWitt
Copy link
Collaborator Author

The diffs should be now clean, only the imports are still an open problem.

@monperrus monperrus merged commit f43586b into INRIA:master Feb 15, 2022
raghav-deepsource pushed a commit to DeepSourceCorp/spoon that referenced this pull request Apr 5, 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.

4 participants