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

Dump Goal for Maven Plugin #107

Merged
merged 8 commits into from Nov 15, 2013

Conversation

Projects
None yet
3 participants
@marchof
Member

marchof commented Nov 10, 2013

I'm new to GitHub, and I don't know if this is the right place to do this, so please, don't crucify me.

I don't know what's your opinion about this, but I think it would be interesting to have Maven goal on Jacoco Maven plugin (something like "remote-dump") that does the same as the Dump Ant task. A new Maven goal would suppress the need to use the Maven Antrun Plugin (and the need to do an Ant script).

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 21, 2013

Member

Pull request #140 addresses this. So closing this one.

Member

marchof commented Sep 21, 2013

Pull request #140 addresses this. So closing this one.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 21, 2013

Member

Re-open as the pull request #140 has been dropped.

Member

marchof commented Sep 21, 2013

Re-open as the pull request #140 has been dropped.

@marchof marchof reopened this Sep 21, 2013

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Oct 3, 2013

Member

@Godin Any chance that you attach your implementation here?

Member

marchof commented Oct 3, 2013

@Godin Any chance that you attach your implementation here?

@ghost ghost assigned Godin Oct 3, 2013

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Oct 27, 2013

Member

@Godin Did you had a chance to work on tests for the dump goal? We got another PR #149, unfortunately without any tests.

Member

marchof commented Oct 27, 2013

@Godin Did you had a chance to work on tests for the dump goal? We got another PR #149, unfortunately without any tests.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Oct 28, 2013

Member

@Godin Regarding IT: Do you have an idea how we could spawn the test subject process in Maven? From what I have found embedding an Ant exec task seems to be the only solution.

Member

marchof commented Oct 28, 2013

@Godin Regarding IT: Do you have an idea how we could spawn the test subject process in Maven? From what I have found embedding an Ant exec task seems to be the only solution.

@ghost ghost assigned marchof Nov 10, 2013

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 10, 2013

Member

@Godin After I finally managed to create a proper integration test for this I decided to push my implementation. Can you please review this?

Note that I needed to add an new feature to the dump goal (also to the Ant task) to make the tests relyable: A new property "retryCount" (10 by default) specifies the number of connection retries. Otherwise the dump goal fails from time to time if the target VM is not yet ready. We've seen this several time before with our CI builds for the Ant task. This should now also be fixed.

Member

marchof commented Nov 10, 2013

@Godin After I finally managed to create a proper integration test for this I decided to push my implementation. Can you please review this?

Note that I needed to add an new feature to the dump goal (also to the Ant task) to make the tests relyable: A new property "retryCount" (10 by default) specifies the number of connection retries. Otherwise the dump goal fails from time to time if the target VM is not yet ready. We've seen this several time before with our CI builds for the Ant task. This should now also be fixed.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 13, 2013

Member

@mfriedenhagen Hi Mirko, may I ask you to review this pull request? Please feel free to add comments or questions.

Member

marchof commented Nov 13, 2013

@mfriedenhagen Hi Mirko, may I ask you to review this pull request? Please feel free to add comments or questions.

private Socket tryConnect() throws IOException {
final InetAddress inetAddress = InetAddress.getByName(address);
int count = 0;
while (true) {

This comment has been minimized.

@mfriedenhagen

mfriedenhagen Nov 14, 2013

Member

I would prefer for (int count = 0; count <= retryCount; count++)

@mfriedenhagen

mfriedenhagen Nov 14, 2013

Member

I would prefer for (int count = 0; count <= retryCount; count++)

This comment has been minimized.

@marchof

marchof Nov 14, 2013

Member

Me too, but unfortunately we need to bail out with the catched exception if we exceed retryCount. Can you show a complete snippet?

@marchof

marchof Nov 14, 2013

Member

Me too, but unfortunately we need to bail out with the catched exception if we exceed retryCount. Can you show a complete snippet?

@mfriedenhagen

This comment has been minimized.

Show comment
Hide comment
@mfriedenhagen

mfriedenhagen Nov 14, 2013

Member

All in all I see a lot of logic duplication between the DumpMojo and the DumpTask which should be extracted IMO in a helper class.

Member

mfriedenhagen commented Nov 14, 2013

All in all I see a lot of logic duplication between the DumpMojo and the DumpTask which should be extracted IMO in a helper class.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 15, 2013

Member

I'll fix the duplicate logic in another PR. For example with ExecFileLoader we already have a shared tool. I'm thinking of a org.jacoco.core.tools package.

Member

marchof commented Nov 15, 2013

I'll fix the duplicate logic in another PR. For example with ExecFileLoader we already have a shared tool. I'm thinking of a org.jacoco.core.tools package.

marchof added a commit that referenced this pull request Nov 15, 2013

@marchof marchof merged commit 962a13d into master Nov 15, 2013

1 check passed

default The Travis CI build passed
Details

@marchof marchof deleted the issue-107 branch Nov 15, 2013

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.