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

Extract Component - Generate Module Extract list #37

Closed
rnveach opened this issue Jun 25, 2017 · 13 comments
Closed

Extract Component - Generate Module Extract list #37

rnveach opened this issue Jun 25, 2017 · 13 comments
Assignees

Comments

@rnveach
Copy link
Member

rnveach commented Jun 25, 2017

Taken from discussions in #29 ,

We need to generate a list of modules, properties, and hierarchy from checkstyle PR for our analysis and to identify full regression model needed.

This the list of information we need right now:

-- Module Information

Package
Class Name
Class hierarchy: Can contain multiple classes. If AbstractSuperCheck is modified, we need to determine that SuperCloneCheck and SuperFinalizeCheck need regression. Must contain full class path.
Interfaces: Same reason as class heirarchy.
Parent Module: Checker or TreeWalker
List of properties

-- Property Information

Name
Type
Default Value
Acceptable Tokens (only for Java and Javadoc checks)

Based on discussions at #19 (comment) and in issue:

As of right now, this probably has to be loaded as a file from a separate maven process.

invoke some simple program in our PR copy of Checkstyle through maven and have it generate the results for us that we need. It will pass the information back to our regression-tool's JVM by a file. We don't need the actual class in memory, we just need it's meta-information.

So we don't have to inject gson dependency into Checkstyle too, I think we should just write out file by hand.
It is ok if we can't cover external maven command by coverage or UT for now.

This is blocked by #19 .

@rnveach
Copy link
Member Author

rnveach commented Jun 26, 2017

Is this component only for json related code and named as json?

This component should be between the components git and module.

It should contain only code relating to everything needed to execute the external maven command and read the Checkstyle extract file produced into memory. If file failed to be created, we should throw a FileNotFoundException, with any additional information if possible.

I am currently preferring the name extract. Is this ok?

@rnveach rnveach changed the title Generate Module Extract list Extract Component - Generate Module Extract list Jun 26, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jun 26, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jun 27, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jun 27, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jun 27, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jun 27, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jun 28, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jun 28, 2017
rnveach pushed a commit that referenced this issue Jun 28, 2017
@Luolc
Copy link
Contributor

Luolc commented Jun 28, 2017

In #40 we completed the work of getting the info for reader of json file and putting the info to memory.

We also need to implement the execution of checkstyle project maven process to generate the extract info file, and fetch the contents of it.

@Luolc
Copy link
Contributor

Luolc commented Jul 5, 2017

Should we create an issue at checkstyle repo for the maven process? I don't have a clear idea how to create a file in a maven life time. I could only imagine to do that in an UT and the file could be generate with running a test by now.

@rnveach
Copy link
Member Author

rnveach commented Jul 5, 2017

@Luolc There will be no changes in Checkstyle for this.
We will maintain the extract program in our repository. CS maintainers don't want this in main repo for now.
It should be compilable, and can be based off the release version of Checkstyle for now. We may have to support backwards compatibility for #15 and some small type of forward compatibility for what might change in a PR.

These are the steps I am seeing for the process:

  1. Clone checkstyle repo if not already. We will need to decide where to store it, and we will need to probably keep it up to date by doing a fetch (probably needs to be skippable for offline mode). User will give us repository location and branch name.
  2. Move extract program to cloned checkstyle repo in src/test/java.
  3. 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.
  4. Examine extract file and read it into memory.
  5. Clean up repository by deleting extract program and extract file.

If you have a better process, let me know.

@Luolc
Copy link
Contributor

Luolc commented Jul 5, 2017

I am fine with these steps. In fact the only difference between this and our previous plan is that the extract program is not put in CS project.

Let's go into the detail.

We will need to decide where to store it

My suggestion: in a extra-resources folder or smth like that. BTW, the groovy script, repositories for the script to test on could be placed here as well, maybe.

we will need to probably keep it up to date by doing a fetch

I think we could just use the PR branch version. Our goal is to test specific PR branch, so the extract infos should describe exactly about the PR branch version.

Edit: In a offline mode, it is sure user would have the PR branch version CS project. We could implement a method that take that repo path and branch name as parameter, make a copy to our project, and do the rest steps.

@rnveach
Copy link
Member Author

rnveach commented Jul 5, 2017

I think we could just use the PR branch version.

Your right, I forgot we were going to do this.
In this case, we don't need to do a clone or store it ourselves. User will give us repository location and branch name. So step 1 can be removed, all other steps should stay.

make a copy to our project

I don't think we need to do a copy. We are only injecting 1 file and output should go under our repository somewhere. We can delete the injected file after we are done as part of a new step 5.

@rnveach
Copy link
Member Author

rnveach commented Jul 5, 2017

@Luolc Please update Issue #32 with list of arguments we find we need to add to it so we don't forget.

@Luolc
Copy link
Contributor

Luolc commented Jul 5, 2017

I don't think we need to do a copy. We are only injecting 1 file and output should go under our repository somewhere. We can delete the injected file after we are done as part of a new step 5.

OK, I am fine with this.

Arguments commented in #32 .

@Luolc
Copy link
Contributor

Luolc commented Jul 6, 2017

Our steps could be split into two separate work:

  • The extract generation program.
  • Codes about moving the program to checkstyle repo, invoking maven command, reading it, cleaning the repo.

I would like to send a PR of the second, only contains the abstract design thinking and discuss the code structure in the PR. I find it difficult to describe my thinking in words, "show me the code" way may be more effective. Edit: #50

And I would work on the first in the meantime.

Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 6, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 6, 2017
@Luolc Luolc mentioned this issue Jul 6, 2017
7 tasks
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 10, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 10, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 10, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 10, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 10, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 11, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 11, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 11, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 11, 2017
rnveach pushed a commit that referenced this issue Jul 11, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 11, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 12, 2017
Luolc added a commit to Luolc/regression-tool that referenced this issue Jul 13, 2017
@rnveach
Copy link
Member Author

rnveach commented Jul 19, 2017

@Luolc Is there anything left to be done in this issue?

@Luolc
Copy link
Contributor

Luolc commented Jul 19, 2017

@rnveach It's done.

Before integrating this into Main, we are not easy to find other bugs.

@rnveach
Copy link
Member Author

rnveach commented Jul 19, 2017

Then I am closing this issue

@rnveach rnveach closed this as completed Jul 19, 2017
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

No branches or pull requests

2 participants