Issue #37: inject, invoke maven command, read file and clean up #50

Luolc · 2017-07-06T17:50:20Z

#37

This PR is just showing the abstract design thinking as mentioned at #37 (comment)

We need do the following things:

run Git command to checkout to the PR branch.
move the extract program to checkstyle repo
invoke maven command
read the file to memory
clear

Problems:

Where to put step 1? According to our design of package, there would be no JGit dependency outside git. So should we create a util class in git and use it in extract? If yes, we already have a GitUtils in test scope, we might need to rename it to smth like GitTestUtils and create a GitUtils for production code. If confirmed, we should make split issue for it and discuss there.
CheckstyleInjector is a temporary name. I haven't thought out a proper name currently. Please share your suggestion if you have good ideas of the name.
I would like to keep getModuleExtractInfosFromReader in ExtractInfoProcessor public, for we need this middleware method to create fake infos in ModuleInfoCollectorTest and ModuleUtilsTest.

rnveach · 2017-07-06T18:18:15Z

Where to put step 1?
there would be no JGit dependency outside git.

truthfully I was thinking we would run a command line git execution instead of using JGit.
I like the idea of using JGit to make it truly system independent and not relying on something being installed.
If we are doing this for JGit, it is possible to do this for Maven too. See maven-embedder.

So should we create a util class in git and use it in extract?

I didn't want to allow jgit everywhere as then someone will import it in the wrong place and think it is acceptable when it isn't. Import control is to restrict bad designs, not to limit our design possibilities.
I don't think one component should be calling another, which is what a utility class in git will look like.
We can either allow jgit inside extract component (I think I prefer this right now), or make a util package and name class something like GitUtil.

CheckstyleInjector

Edit: I didn't look at source changes first.
Name sounds fine to me for now.

I would like to keep getModuleExtractInfosFromReader in ExtractInfoProcessor public, for we need this middleware method to create fake infos in ModuleInfoCollectorTest and ModuleUtilsTest.

No. If something is needed for testing, we should use reflection and break class that way than leave internal structure open for someone to mis-use. Production code should not be defined by tests except for maybe splitting things into smaller pieces.
In my work, if I need a method like this, I use reflection and store it as a class field for use everywhere in test.

Luolc · 2017-07-06T18:39:43Z

No. If something is needed for testing, we should use reflection and break class that way than leave internal structure open for someone to mis-use.

OK.

We can either allow jgit inside extract component (I think I prefer this right now)

I agree with this.

it is possible to do this for Maven too. See maven-embedder

Ya, I was thinking to use it. Already been investigating on it.

Luolc · 2017-07-10T14:39:27Z

@rnveach Updated.
missing implementation of invokeMavenCommand currently.

I don't use static method in CheckstyleInjector this time, for every method in it needs repoPath and branch, and we need close repository, I think an instance is better.

Maybe we need to combine the another part of this issue then we could make test for all this updates.

rnveach

Are we going to finish this PR?

rnveach · 2017-07-11T10:50:34Z

src/main/java/com/github/checkstyle/regression/extract/CheckstyleInjector.java

+        checkoutToPrBranch();
+        copyInjectFilesToCheckstyleRepo();
+        invokeMavenCommand();
+        return null;


Needs a result.

rnveach · 2017-07-11T10:51:15Z

src/main/java/com/github/checkstyle/regression/extract/CheckstyleInjector.java

+     * Invokes Maven command to generate the extract info file in checkstyle repository.
+     */
+    private static void invokeMavenCommand() {
+        // invoke maven command to generate the extract info file


needs to do something.

Updated. But have some problems. See my comments below.

rnveach · 2017-07-11T10:53:00Z

pom.xml

+                                    <regex>
+                                        <pattern>com.github.checkstyle.regression.extract.CheckstyleInjector</pattern>
+                                        <branchRate>0</branchRate>
+                                        <lineRate>0</lineRate>


Please create issue to create some tests and coverage for this class at a later time.

Luolc · 2017-07-11T16:45:25Z

@rnveach I encountered some strange problems:

cobertura failed. I add/update the regex pattern according to the html report, but it still failed.
If I uncomment the UT added in ExtractInfoProcessorTest: https://github.com/Luolc/regression-tool/blob/689f22c5d44223c268fe23227b8311bdae039e26/src/test/java/com/github/checkstyle/regression/extract/ExtractInfoProcessorTest.java#L105, mvn clean test is fine but mvn clean compile cobertura:cobertura would run with a lot of test error.
If comment/remove this UT, then there would be no error with compile cobertura. I guess it is caused by System.setProperty(PROPERTY_KEY_DIRECTORY, repoPath);, and this is because the problem described in https://stackoverflow.com/questions/33400574/unable-to-run-maven-tasks-through-mavencli-maven-embedder. Can't fix this after a long time of investigation. :(

rnveach · 2017-07-11T19:08:07Z

cobertura failed

remove quiet from POM for cobertura and you get this error:

[ERROR] com.github.checkstyle.regression.extract.CheckstyleInjector$1 failed coverage check. Line coverage rate of 0.0% is below 100.0%

Cobertura sees main class and inner classes differently.
There was a reason this is enabled in CS, feel free to make another PR to remove it if you wish.

Change line to something like: <pattern>com.github.checkstyle.regression.extract.CheckstyleInjector.*</pattern>

rnveach · 2017-07-11T19:16:02Z

If I uncomment the UT added in ExtractInfoProcessorTest

This is why I have said repeatedly, lets not worry about code coverage on cloning, maven, etc...
Isolate it as best as you can and mock its true run away. We will eventually use travis to make sure it functions properly in a full run.
I don't want junits doing actual cloning or running maven/git commands except on our own sources or repositories we can quickly create.

guess it is caused by System.setProperty(PROPERTY_KEY_DIRECTORY, repoPath);

Why not set this as arguments to doMain as specified in https://stackoverflow.com/questions/33400574/unable-to-run-maven-tasks-through-mavencli-maven-embedder
Ex: int result = cli.doMain(new String[] { "-Dmaven.multiModuleProjectDirectory=" + projectRoot, "install" }, "/path/to/project", System.out, System.err);

Luolc · 2017-07-12T00:57:50Z

We will eventually use travis to make sure it functions properly in a full run.

I add this in UT just for quick test and debug, I would remove it when I confirm the code function is acceptable.

Why not set this as arguments to doMain as specified in

I tried this but it doesn't work. Still has error -Dmaven.multiModuleProjectDirectory system property is not set. Check $M2_HOME environment variable and mvn script match.

Luolc · 2017-07-12T01:10:31Z

Change line to something like: com.github.checkstyle.regression.extract.CheckstyleInjector.*

It works. Thanks. :)

rnveach · 2017-07-12T12:44:45Z

src/test/java/com/github/checkstyle/regression/extract/ExtractInfoProcessorTest.java

        }
    }
+
+    //@Test


Do not leave commented code in.

rnveach · 2017-07-12T12:48:38Z

src/main/java/com/github/checkstyle/regression/extract/ExtractInfoProcessor.java

+                returnValue = getModuleExtractInfosFromReader(reader);
+            }
+            catch (IOException ex) {
+                throw new InjectException("unable to read to info file", ex);


This message is not very informative of issue.
Expand on what we were trying to do. Since it is the extract file, it should be in message somewhere.

rnveach · 2017-07-12T12:48:43Z

src/main/java/com/github/checkstyle/regression/extract/ExtractInfoProcessor.java

+
+        try {
+            try {
+                final File file = injector.generateExtractInfoFile();


if generateExtractInfoFile doesn't throw an IOException it should be outside the 2nd level try.

rnveach · 2017-07-12T12:50:24Z

pom.xml

+                                    <regex>
+                                        <pattern>com.github.checkstyle.regression.extract.InjectException</pattern>
+                                        <branchRate>0</branchRate>
+                                        <lineRate>0</lineRate>


Needs issue.

rnveach · 2017-07-12T12:50:33Z

pom.xml

+                                    <regex>
+                                        <pattern>com.github.checkstyle.regression.extract.CheckstyleInjector.*</pattern>
+                                        <branchRate>0</branchRate>
+                                        <lineRate>0</lineRate>


Needs issue.

…n up

rnveach

I am not seeing anything.

Luolc mentioned this pull request Jul 6, 2017

Extract Component - Generate Module Extract list #37

Closed

Luolc mentioned this pull request Jul 6, 2017

Issue #37: extract program #52

Merged

7 tasks

Luolc force-pushed the issue37-1 branch from e05140e to 639e730 Compare July 10, 2017 14:19

Luolc force-pushed the issue37-1 branch from 639e730 to d61fe59 Compare July 11, 2017 04:50

rnveach requested changes Jul 11, 2017

View reviewed changes

Luolc force-pushed the issue37-1 branch from d61fe59 to 689f22c Compare July 11, 2017 16:37

Luolc force-pushed the issue37-1 branch from 689f22c to c8a65e6 Compare July 12, 2017 01:06

rnveach requested changes Jul 12, 2017

View reviewed changes

Issue checkstyle#37: inject, invoke maven command, read file and clea…

241fa66

…n up

Luolc force-pushed the issue37-1 branch from c8a65e6 to 241fa66 Compare July 13, 2017 08:47

Luolc mentioned this pull request Jul 13, 2017

Codecov: CheckstyleInjector, InjectException #54

Open

rnveach approved these changes Jul 19, 2017

View reviewed changes

rnveach merged commit f1f6c45 into checkstyle:master Jul 19, 2017

Luolc deleted the issue37-1 branch July 19, 2017 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #37: inject, invoke maven command, read file and clean up #50

Issue #37: inject, invoke maven command, read file and clean up #50

Luolc commented Jul 6, 2017

rnveach commented Jul 6, 2017 •

edited

Loading

Luolc commented Jul 6, 2017

Luolc commented Jul 10, 2017

rnveach left a comment

rnveach Jul 11, 2017

Luolc Jul 11, 2017

rnveach Jul 11, 2017

Luolc Jul 11, 2017

rnveach Jul 11, 2017

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017 •

edited

Loading

rnveach commented Jul 11, 2017 •

edited

Loading

Luolc commented Jul 12, 2017

Luolc commented Jul 12, 2017

rnveach Jul 12, 2017

Luolc Jul 13, 2017

rnveach Jul 12, 2017

Luolc Jul 13, 2017

rnveach Jul 12, 2017

Luolc Jul 13, 2017

rnveach Jul 12, 2017

Luolc Jul 13, 2017

rnveach Jul 12, 2017

Luolc Jul 13, 2017

rnveach left a comment

                       }
                   }
+                  //@Test

Issue #37: inject, invoke maven command, read file and clean up #50

Issue #37: inject, invoke maven command, read file and clean up #50

Conversation

Luolc commented Jul 6, 2017

rnveach commented Jul 6, 2017 • edited Loading

Luolc commented Jul 6, 2017

Luolc commented Jul 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luolc commented Jul 11, 2017

rnveach commented Jul 11, 2017 • edited Loading

rnveach commented Jul 11, 2017 • edited Loading

Luolc commented Jul 12, 2017

Luolc commented Jul 12, 2017

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

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 left a comment

Choose a reason for hiding this comment

rnveach commented Jul 6, 2017 •

edited

Loading

rnveach commented Jul 11, 2017 •

edited

Loading

rnveach commented Jul 11, 2017 •

edited

Loading