Issue #37: extract program #52

Luolc · 2017-07-06T18:52:37Z

This is the file that would be injected into checkstyle project.

Also we should update pom.xml in checkstyle to add a Gson dependency.

As mentioned at https://github.com/checkstyle/regression-tool/pulls#issuecomment-310935967, we ignored the last two for now.

Edit: If necessary, I am fine with combining this PR with #50 into one PR together.

rnveach · 2017-07-06T19:00:14Z

pom.xml in checkstyle should be added a Gson dependency.

Checkstyle doesn't want any changes in their repository for our process, so this won't be possible.

See my comment at #37 (comment) :

So we don't have to inject gson dependency into Checkstyle too, I think we should just write out file by hand.

Otherwise, we have to inject test dependency into their POM, which may not be pretty and require us undoing the change after our process.

rnveach

We need a way to ensure this file is compilable, especially in checkstyle process.
I suggest we use a new travis item where we clone checkstyle, move our file there, and just test for success on mvn compile.
We can go further and test execution of file to make sure it doesn't atleast throw exception or fail.

rnveach · 2017-07-06T19:04:47Z

src/main/resources/com/github/checkstyle/regression/extract/ExtractInfoGeneratorTest.java

+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonArray;
+import com.google.gson.JsonObject;


See my comments.

rnveach · 2017-07-06T19:33:00Z

src/main/resources/com/github/checkstyle/regression/extract/ExtractInfoGeneratorTest.java

+    @Test
+    public void generateExtractInfoFile() throws IOException {
+        final Gson gson = new GsonBuilder().setPrettyPrinting().create();
+        final List<JsonObject> objects = CheckUtil.getCheckstyleModules().stream()


One issue with CheckUtil.getCheckstyleModules is we can't test on old commits.
I think we should find someway to be more backward compatible. This can be a new issue.

rnveach · 2017-07-06T19:33:56Z

src/main/resources/com/github/checkstyle/regression/extract/ExtractInfoGeneratorTest.java

+        if (ModuleReflectionUtils.isCheckstyleCheck(clazz)) {
+            parent = "TreeWalker";
+        }
+        else if ("Checker".equals(name)) {


Check if it is a root module instead of Checker.

Luolc · 2017-07-07T02:30:37Z

@rnveach

Checkstyle doesn't want any changes in their repository for our process, so this won't be possible.
Otherwise, we have to inject test dependency into their POM, which may not be pretty and require us undoing the change after our process.

I don't mean that we need change in checkstyle main repo. At the meantime we do the injection, we just need to find this line, https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L267, and add the dependency below the line. If pom.xml changes one day, the maven process would fail quickly and then we would fix it. And I think this way is much better than we generate the file by our hand.

All the clear up operation can be done with a simple git checkout ., whatever JGit or just running command line Git. So we don't need to be worried about clearing.

rnveach · 2017-07-07T03:15:25Z

If pom.xml changes one day, the maven process would fail quickly and then we would fix it.

Whereas if we write file by hand, it will never fail.
What are we gaining by adding this dependency besides easier to make/maintain code?
Adding dependency will make it harder to test in a travis build as travis was going to be a separate process from our production code.

Luolc · 2017-07-10T11:04:36Z

@rnveach Updated with our own json generation codes.

rnveach · 2017-07-10T13:24:37Z

We need a way to ensure this file is compilable, especially in checkstyle process.

@Luolc Can you update travis to make sure this code compiles and executes without exception?

Luolc · 2017-07-10T14:41:57Z

@rnveach Done.

rnveach · 2017-07-10T15:14:19Z

.travis.yml

+        - CMD5="checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/Json.java &&"
+        - CMD6="cd checkstyle &&"
+        - CMD7="mvn clean compile"
+        - CMD="$CMD1$CMD2$CMD3$CMD4$CMD5$CMD6$CMD7"


We can go further and test execution of file to make sure it doesn't atleast throw exception or fail.

What about this statement?

Maybe better in #50 ? The test on generated file needs a lot of process of read/convert/... And many logic are included in #50 . Should we combine the changes of these two PR and close one of them?

I am fine with this being separated.
I don't expect this to be done in #50 in the junits (if that was what your thinking), as junits won't be cloning the external repository as part of their tests. They should be pretty quick tests, which is why missing code coverage for this one script is fine. That is why travis makes better sense for doing all these long complicated tasks.

Once you add this to travis I will merge this PR as it will be confirmed working.

What level of the test should be? The ones I could think out now:

check it is a valid json

check whether it is in correct format(a list, each item contains "name", "packageName" etc)
Do you have anything to add?

And, to test a json format is much hard to do with only shell script, if we won't do it in UT, then could we use other script lang like Groovy?

@Luolc no exception test is fine for now in travis.
Travis won't be verifying output for now. We can manually confirm this.

Later since this extract program should work on all checkstyle versions, we could try to use release version for code coverage and expected output in another issue.
I don't want to put something like this with full cloning in JUnits.
We may have to import Checkstyle release's test JAR dependency to accomplish this.

@rnveach Don't exactly understand the way actually...
But let's do the further discussion in the issue for this, if you think current test is fine for now. :) Should I create that issue now?

rnveach · 2017-07-10T15:15:05Z

src/main/resources/com/github/checkstyle/regression/extract/Json.java

+ * Our own Json processing classes.
+ * @author LuoLiangchen
+ */
+class Json {


rnveach · 2017-07-10T15:15:23Z

src/main/resources/com/github/checkstyle/regression/extract/Json.java

+import java.util.stream.Collectors;
+
+/**
+ * Our own Json processing classes.


processing classes => processing utility.

rnveach · 2017-07-10T17:14:14Z

@Luolc One final thing, can you upload the result of this process somewhere for us to look at during GSoC?
Please name it after commit hash from CS project you use it on.

Luolc · 2017-07-11T01:06:29Z

@rnveach Do you mean upload it every time sending a PR? I may try to add a deploy operation in Travis.

rnveach · 2017-07-11T01:59:29Z

@Luolc I meant a one time upload in a place where we can look back on it in the future.
Every PR is not needed.

http://textuploader.com/ or something similar maybe?

Luolc · 2017-07-11T02:25:03Z

@rnveach I upload a gist: https://gist.github.com/Luolc/f3898dc507d66d21b48d85c1c09e5001

rnveach · 2017-07-11T02:48:14Z

Don't exactly understand the way actually...

If you mena the way to execute the test to make sure it passes with no errors, see #37 (comment)

Execute extract program as a one time junit. See https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/64d616821b2d2265fb8291fd6950b4eef83c4547/.travis.yml#L68 on how to do this execution.

It should literally just be && mvn test -Dtest=ExtractInfoGeneratorTest#generateExtractInfoFile

Luolc · 2017-07-11T02:55:23Z

@rnveach Oh I think I misunderstood this. I was thinking to test the json format. If you mean run this UT, I am clear of the way.

rnveach · 2017-07-11T03:02:53Z

I assume you already tested the json format since you had it originally in our repo with the full thing but than trimmed it down for just what we used in tests.
What is in gist can also be tested by itself.

rnveach

missed this before.

rnveach · 2017-07-11T03:03:54Z

src/main/resources/com/github/checkstyle/regression/extract/ExtractInfoGeneratorTest.java

+        if (ModuleReflectionUtils.isCheckstyleCheck(clazz)) {
+            parent = "TreeWalker";
+        }
+        else if (RootModule.class.isAssignableFrom(clazz)) {


RootModule.class.isAssignableFrom => ModuleReflectionUtils.isRootModule

rnveach requested changes Jul 6, 2017

View reviewed changes

Luolc force-pushed the issue37-2 branch 2 times, most recently from c711cf3 to e083a53 Compare July 10, 2017 10:40

Luolc force-pushed the issue37-2 branch from e083a53 to 93d7b29 Compare July 10, 2017 14:32

rnveach requested changes Jul 10, 2017

View reviewed changes

Luolc force-pushed the issue37-2 branch from 93d7b29 to 55ff1ba Compare July 10, 2017 15:46

Luolc force-pushed the issue37-2 branch from 55ff1ba to bd14a2a Compare July 11, 2017 02:57

rnveach requested changes Jul 11, 2017

View reviewed changes

Luolc force-pushed the issue37-2 branch from bd14a2a to 65e4f30 Compare July 11, 2017 03:52

Issue checkstyle#37: extract program

57a825e

Luolc force-pushed the issue37-2 branch from 65e4f30 to 57a825e Compare July 11, 2017 04:50

rnveach merged commit 000ef03 into checkstyle:master Jul 11, 2017

Luolc deleted the issue37-2 branch July 11, 2017 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #37: extract program #52

Issue #37: extract program #52

Luolc commented Jul 6, 2017 •

edited

Loading

rnveach commented Jul 6, 2017

rnveach left a comment •

edited

Loading

rnveach Jul 6, 2017

rnveach Jul 6, 2017

rnveach Jul 6, 2017

Luolc Jul 10, 2017

Luolc commented Jul 7, 2017 •

edited

Loading

rnveach commented Jul 7, 2017

Luolc commented Jul 10, 2017

rnveach commented Jul 10, 2017

Luolc commented Jul 10, 2017

rnveach Jul 10, 2017

Luolc Jul 10, 2017

rnveach Jul 10, 2017 •

edited

Loading

Luolc Jul 11, 2017

rnveach Jul 11, 2017 •

edited

Loading

Luolc Jul 11, 2017

rnveach Jul 10, 2017

Luolc Jul 10, 2017

rnveach Jul 10, 2017

Luolc Jul 10, 2017

rnveach commented Jul 10, 2017 •

edited

Loading

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017 •

edited

Loading

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017 •

edited

Loading

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017

rnveach left a comment

rnveach Jul 11, 2017

Luolc Jul 11, 2017

Issue #37: extract program #52

Issue #37: extract program #52

Conversation

Luolc commented Jul 6, 2017 • edited Loading

rnveach commented Jul 6, 2017

rnveach left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 7, 2017 • edited Loading

rnveach commented Jul 7, 2017

Luolc commented Jul 10, 2017

rnveach commented Jul 10, 2017

Luolc commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach commented Jul 10, 2017 • edited Loading

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017 • edited Loading

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017 • edited Loading

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 6, 2017 •

edited

Loading

rnveach left a comment •

edited

Loading

Luolc commented Jul 7, 2017 •

edited

Loading

rnveach Jul 10, 2017 •

edited

Loading

rnveach Jul 11, 2017 •

edited

Loading

rnveach commented Jul 10, 2017 •

edited

Loading

rnveach commented Jul 11, 2017 •

edited

Loading

rnveach commented Jul 11, 2017 •

edited

Loading