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

ExecDumpClient should report error when no execution data is retrieved #538

Merged
merged 4 commits into from May 24, 2017

Conversation

Projects
None yet
3 participants
@marchof
Member

marchof commented May 22, 2017

Ant and Maven dump commands use the ExecDumpClient class from the org.jacoco.core module. This utility does not evaluate the response status after sending a dump request. If the remote closes the connection without sending a response this is silently ignored.

Expected behavior is that a IOException is thrown when a dump request is not answered.

@marchof marchof self-assigned this May 22, 2017

@marchof marchof referenced this pull request May 22, 2017

Merged

Simple Command Line Interface for JaCoCo #525

5 of 5 tasks complete
@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin May 22, 2017

Member

@marchof one case is obvious - when remote is something that always closes connection; wondering under which conditions real JaCoCo agent can do this?

Member

Godin commented May 22, 2017

@marchof one case is obvious - when remote is something that always closes connection; wondering under which conditions real JaCoCo agent can do this?

@marchof marchof requested a review from Godin May 22, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof May 22, 2017

Member

@Godin No idea how the actual JaCoCo agent would behave like this. This PR is more about making the client bullet-proof especially about missconfiguration.

Member

marchof commented May 22, 2017

@Godin No idea how the actual JaCoCo agent would behave like this. This PR is more about making the client bullet-proof especially about missconfiguration.

Show outdated Hide outdated org.jacoco.doc/docroot/doc/changes.html Outdated
Show outdated Hide outdated org.jacoco.core.test/src/org/jacoco/core/tools/ExecDumpClientTest.java Outdated
@@ -123,7 +123,10 @@ public ExecFileLoader dump(final InetAddress address, final int port)
.setExecutionDataVisitor(loader.getExecutionDataStore());
remoteWriter.visitDumpCommand(dump, reset);
remoteReader.read();
if (!remoteReader.read()) {

This comment has been minimized.

@Godin

Godin May 23, 2017

Member

@marchof we have similar code in org.jacoco.examples/src/org/jacoco/examples/ExecutionDataClient.java. Later we can update it to use ExecDumpClient, but IMO anyway will be better to apply same modification in it with this PR.

@Godin

Godin May 23, 2017

Member

@marchof we have similar code in org.jacoco.examples/src/org/jacoco/examples/ExecutionDataClient.java. Later we can update it to use ExecDumpClient, but IMO anyway will be better to apply same modification in it with this PR.

This comment has been minimized.

@marchof

marchof May 24, 2017

Member

Fixed in acfb4b0

@marchof

marchof May 24, 2017

Member

Fixed in acfb4b0

marchof added some commits May 24, 2017

Improve test case according to @Godin's review comments
* Assert exception message
* Avoid raise condition
* Use new naming scheme

marchof added a commit that referenced this pull request May 24, 2017

@Godin

Godin approved these changes May 24, 2017

@Godin Godin merged commit 10f3ff0 into master May 24, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Godin Godin deleted the issue-538 branch May 24, 2017

@Godin Godin added this to the 0.8.0 milestone May 24, 2017

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin
Member

Godin commented May 24, 2017

@marchof 👍

@denisa

This comment has been minimized.

Show comment
Hide comment
@denisa

denisa Jun 8, 2017

Thanks for having that fixed, I just spend an hour tracking that down on 0.7.9.

My use case:
I have a docker image that contains wildfly and the jacobo agent jar file.
The docker image always exposes port 6300, but I decide upon launching the image if I want to enable the jacoco agent or not.

When I connect to a running docker instance that does not have jacoco enabled, ExecDumpClient.dump() returns an (empty) ExecFileLoader instead of failing as desired.

denisa commented Jun 8, 2017

Thanks for having that fixed, I just spend an hour tracking that down on 0.7.9.

My use case:
I have a docker image that contains wildfly and the jacobo agent jar file.
The docker image always exposes port 6300, but I decide upon launching the image if I want to enable the jacoco agent or not.

When I connect to a running docker instance that does not have jacoco enabled, ExecDumpClient.dump() returns an (empty) ExecFileLoader instead of failing as desired.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jun 8, 2017

Member

@denisa could you please clarify just to be sure - do you mean that after this change in your use case you're getting IOException as expected?

Member

Godin commented Jun 8, 2017

@denisa could you please clarify just to be sure - do you mean that after this change in your use case you're getting IOException as expected?

@denisa

This comment has been minimized.

Show comment
Hide comment
@denisa

denisa Jun 8, 2017

Yes, this bug is fixed: 0.7.10-SNAPSHOT throws an IOException as designed when 0.7.9 simply returned an object.

denisa commented Jun 8, 2017

Yes, this bug is fixed: 0.7.10-SNAPSHOT throws an IOException as designed when 0.7.9 simply returned an object.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Jun 8, 2017

Member

@denisa cool! thank you for information 👍

Member

Godin commented Jun 8, 2017

@denisa cool! thank you for information 👍

@jacoco jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

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