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

[Feat] Add support for pushing the amplification results in a MongoDB database #852

Merged
merged 10 commits into from
Aug 14, 2019
Merged

[Feat] Add support for pushing the amplification results in a MongoDB database #852

merged 10 commits into from
Aug 14, 2019

Conversation

henry-lp
Copy link
Contributor

@henry-lp henry-lp commented Aug 6, 2019

Hi :), original PR (#850) . This contains only the structure of the soon-to-be implemented. Awaiting for review @monperrus @danglotb .

@henry-lp
Copy link
Contributor Author

henry-lp commented Aug 7, 2019

Hi @monperrus :) , I have attempted to resolved it and wait for new comments .

Copy link
Member

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

Thanks for the progress

@monperrus
Copy link
Member

@danglotb we're almost there. WDYT?

@danglotb
Copy link
Member

danglotb commented Aug 8, 2019

Hello, the changes look good to me.

A question: In which format will you push the information in the database?
Could we rely on a single String?

If so, it might be better to have a single method to report the result of test selector, such as reportTestSelectionInformation rather than having multiples methods for each test selector, which might be tedious to maintain. WDYT?

@henry-lp
Copy link
Contributor Author

henry-lp commented Aug 8, 2019

Hi @danglotb , It's in JSON format :) . A single string would need a bit of work to divide it to relevant parts before submitting for the database . How would the String look like ? :) .

@danglotb
Copy link
Member

danglotb commented Aug 8, 2019

It can be the string representation of the JSON then, isn't it?

The test selector would create this JSON with its info, such as it is already done, e.g. in PitMutantScoreSelector, and give the string representation to the collector using the API reportTestSelectionInformation(String s).

We could also improve the eu.stamp_project.utils.report.output.selector.TestClassJSON interface, that is implemented by all the TestClassJSONs classes, such as eu.stamp_project.utils.report.output.selector.mutant.json.TestClassJSON or eu.stamp_project.utils.report.output.selector.coverage.json.TestClassJSON.

Then, reportTestSelectionInformation(String s) would just transform the String into a correct JSON and push it on the DB (or push directly the JSON, depending on the paragraph above).

WDYT?

@henry-lp
Copy link
Contributor Author

henry-lp commented Aug 8, 2019

@danglotb hi :) , yes we can have that . My thoughts, we can have a document taking the saved information in the previous step in the function reportJSONMutants so maybe reportTestSelectionInformation(String s) can be reportTestSelectionInformation() and it will send information in the implementation over to Mongodb. The document differs depending on which selector it was .

I'll make a commit soon and then maybe we can take a look and discuss more :).

@henry-lp
Copy link
Contributor Author

henry-lp commented Aug 8, 2019

@danglotb @monperrus resolved :) . Waiting for new comments.

@danglotb I have added a basic code structure according to my idea about your suggestion. Please have a look :) .

@monperrus
Copy link
Member

OK for me, thanks @tailp. @danglotb could you do the last checks and merge? Thanks!

@danglotb
Copy link
Member

danglotb commented Aug 9, 2019

I relaunched the build on Travis.

I dislike the fact that the JSON classes are using the collector.

For me, the JSON classes are pure data representation and the only available API should be the modification of these data.

I would rather prefer that the collector should be the one that uses the data. But it a conceptual vision of the classes' role.

Questions:

  • Could we implement a collector that pushes the data into a file? Could write some unit tests about this?
  • Do you plan to implement the configuration through the command line in a future PR?

Thank you!

@henry-lp
Copy link
Contributor Author

henry-lp commented Aug 9, 2019

@danglotb hi :), should I revert back to the previous structure proposed by @monperrus or do you prefer to keep this structure ? . The previous structure does not embed code into the JSON classes, which seems to be a better alternative according to your comments. Also better for me since I already more or less make it works with the webinterface and also have some unittests for it.

Pushing it to a file then read it would be a bit round about, while we can just give the infomation directly to the mongodb.

which configuration do you mean ? :) .

@monperrus
Copy link
Member

Could we implement a collector that pushes the data into a file? Could write some unit tests about this?

Yes. Maybe in a subsequent PR?

Do you plan to implement the configuration through the command line in a future PR?

That's an option.

What's the concrete next step to make progress here towards merging this baby PR?

@danglotb
Copy link
Member

Hello, I would like to have the TestClassJSON to be pure data classes, i.e. without any logic such as the call to the Configuration.getInformationCollector().reportSelectorInformation(report);.

Concretely, changes reportTestSelectionInformation into a likewise toString. You can probably use GSON API to format correctly the data into JSON String style rather than by hands.
Puts the logic in the TestSelector, i.e. replaces
testClassJSON.reportTestSelectionInformation();
by something like
Configuration.getInformationCollector().reportSelectorInformation(testClassJSON.toString());

And then, the testClassJSON contains only information and from the point the view of the system, it represents a JSON file.

On my side, I must fix Travis, it seems that it is able to download a dependency:

[WARNING] Could not transfer metadata org.eclipse.platform:org.eclipse.osgi/maven-metadata.xml from/to sonatype-apache (https://repository.apache.org/releases/): Connect to repository.apache.org:443 [repository.apache.org/207.244.88.140] failed: Connection timed out (Connection timed out)

It stucks at this step, trying to download it, again and again, making the build fail.

@henry-lp
Copy link
Contributor Author

@danglotb Sure :) , it's resolved . Please have a look .

@danglotb
Copy link
Member

Hello @tailp.

Good for me, unless that the toString() method should not be declared in the interface and you should add an @Override annotation since you are overriding the toString() from java.lang.Object.

Still have some issues with Travis, dunno how to fix this yet. Sorry.

Thank you!

@henry-lp
Copy link
Contributor Author

@danglotb I understand, no worries :) . The toString() method is now removed from the interface.

@henry-lp
Copy link
Contributor Author

henry-lp commented Aug 14, 2019

@danglotb hi :) , I got this error with Coverall on travis (Somehow it could not find my NullCollector.java)

[ERROR] Failed to execute goal org.eluder.coveralls:coveralls-maven-plugin:4.3.0:report (default-cli) on project dspot: I/O operation failed: No source found for eu/stamp_project/mongodb/NullCollector.java -> [Help 1]

[ERROR] 

[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.

[ERROR] Re-run Maven using the -X switch to enable full debug logging.

[ERROR] 

[ERROR] For more information about the errors and possible solutions, please read the following articles:

[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

The command "./.travis/$SCRIPT $DSPOT_VERSION" exited with 1.

It does not make any sense to me since I have not used Coverall before. This also occured before with MongodbCollector.java but was somehow fixed after your fix :) . Maybe you happens to know the solution to this ? . Thanks .

@danglotb
Copy link
Member

Hello @tailp

I'm looking at this.

@danglotb
Copy link
Member

It is because the used package is package eu.stamp_project.mongodb; while your sources file are in eu/stamp_project/dspot/mongodb.

Please move your sources file, i.e. all the classes of the package package eu.stamp_project.mongodb; to the correct directory.

In addition to this, you can add this package to the package eu.stamp_project.utils , and so having the package eu.stamp_project.utils.mongodb in the directory dspot/src/main/java/eu/stamp_project/utils/mongodb.

You may also want put all the generic class into a package that is more generic, such as eu.stamp_project.utils.collector and put the mongodb specific classes into eu.stamp_project.utils.collector.mongodb in order to have a better hierarchy of the packages/files.

@henry-lp
Copy link
Contributor Author

henry-lp commented Aug 14, 2019

It is because the used package is package eu.stamp_project.mongodb; while your sources file are in eu/stamp_project/dspot/mongodb.

Please move your sources file, i.e. all the classes of the package package eu.stamp_project.mongodb; to the correct directory.

In addition to this, you can add this package to the package eu.stamp_project.utils , and so having the package eu.stamp_project.utils.mongodb in the directory dspot/src/main/java/eu/stamp_project/utils/mongodb.

You may also want put all the generic class into a package that is more generic, such as eu.stamp_project.utils.collector and put the mongodb specific classes into eu.stamp_project.utils.collector.mongodb in order to have a better hierarchy of the packages/files.

Thank you very much :) . I'll try fixing it now once again .

(Ready to merge again when creating a generic collector package)

@danglotb
Copy link
Member

Ready to merge for me. @tailp All good?

@henry-lp
Copy link
Contributor Author

@danglotb yes :) .

@danglotb danglotb merged commit c96221d into STAMP-project:master Aug 14, 2019
@henry-lp henry-lp deleted the Mongodb-ForMerge2 branch August 14, 2019 13:50
@coveralls
Copy link

coveralls commented Aug 14, 2019

Pull Request Test Coverage Report for Build 2180

  • 37 of 41 (90.24%) changed or added relevant lines in 10 files are covered.
  • 12 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 84.589%

Changes Missing Coverage Covered Lines Changed/Added Lines %
dspot/src/main/java/eu/stamp_project/utils/report/output/selector/change/json/TestClassJSON.java 0 1 0.0%
dspot/src/main/java/eu/stamp_project/utils/collector/NullCollector.java 1 4 25.0%
Files with Coverage Reduction New Missed Lines %
dspot/src/main/java/eu/stamp_project/dspot/Amplification.java 12 85.71%
Totals Coverage Status
Change from base Build 2107: 0.3%
Covered Lines: 4973
Relevant Lines: 5879

💛 - Coveralls

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