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

Add Artifact.SCOPE_PROVIDED artifacts to aggregate report #572

Merged
merged 1 commit into from Aug 10, 2017

Conversation

Projects
None yet
3 participants
@chonton
Contributor

chonton commented Aug 2, 2017

I have an aggregate maven project where one of the projects must be scoped as 'provided'. I would like it to be still considered for inclusion in the aggregate report.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 2, 2017

Member

@chonton Thanks for the contribution! May I ask you for your use case? Especially it looks like my current assumptions do not hold true:

  1. A separate project only for the purpose of creating the coverage report is required. In your case it looks like you can rely on a existing project.
  2. My understanding of usage of scope PROVIDED is a dependency on a common API (like Servlet) which is typically not part of the reactor itself. Our goal can only create reports for projects from the reactor.
Member

marchof commented Aug 2, 2017

@chonton Thanks for the contribution! May I ask you for your use case? Especially it looks like my current assumptions do not hold true:

  1. A separate project only for the purpose of creating the coverage report is required. In your case it looks like you can rely on a existing project.
  2. My understanding of usage of scope PROVIDED is a dependency on a common API (like Servlet) which is typically not part of the reactor itself. Our goal can only create reports for projects from the reactor.
@chonton

This comment has been minimized.

Show comment
Hide comment
@chonton

chonton Aug 2, 2017

Contributor

My typical reactor project has three modules:

  • api (interface jar)
  • service (implementation delivered as a docker image)
  • test (integration tests in a docker image)

Both service and test have a compile dependency on api. Additionally, test has a particular dependency on service; the service module must be built before test so that service docker image can be launched during pre-integration-test phase, and the classes from service should not be included in the compile or runtime classpath of test.

To prevent the service classes from being included in the test docker image, the dependency cannot be compile or runtime. The jacoco aggregate goal uses test to distinguish the projects that provide coverage without providing code.

This leaves provided, which seems correct, as the service is providing the implementation of api.

Contributor

chonton commented Aug 2, 2017

My typical reactor project has three modules:

  • api (interface jar)
  • service (implementation delivered as a docker image)
  • test (integration tests in a docker image)

Both service and test have a compile dependency on api. Additionally, test has a particular dependency on service; the service module must be built before test so that service docker image can be launched during pre-integration-test phase, and the classes from service should not be included in the compile or runtime classpath of test.

To prevent the service classes from being included in the test docker image, the dependency cannot be compile or runtime. The jacoco aggregate goal uses test to distinguish the projects that provide coverage without providing code.

This leaves provided, which seems correct, as the service is providing the implementation of api.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 2, 2017

Member

@chonton Indeed, this is nice use case where the aggregate-report works well. Thx!

@Godin Do you agree to merge this? If so I we need to add a change log.

Member

marchof commented Aug 2, 2017

@chonton Indeed, this is nice use case where the aggregate-report works well. Thx!

@Godin Do you agree to merge this? If so I we need to add a change log.

@chonton

This comment has been minimized.

Show comment
Hide comment
@chonton

chonton Aug 7, 2017

Contributor

@marchof and @Godin, anything else required to merge this PR?

Contributor

chonton commented Aug 7, 2017

@marchof and @Godin, anything else required to merge this PR?

@Godin Godin self-assigned this Aug 8, 2017

@Godin Godin added this to the 0.8.0 milestone Aug 8, 2017

@chonton

This comment has been minimized.

Show comment
Hide comment
@chonton

chonton Aug 10, 2017

Contributor

anything else needed to merge this PR?

Contributor

chonton commented Aug 10, 2017

anything else needed to merge this PR?

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Aug 10, 2017

Member

anything else needed to merge this PR?

@chonton not so much - I just need to come from vacation back to work 😉

Member

Godin commented Aug 10, 2017

anything else needed to merge this PR?

@chonton not so much - I just need to come from vacation back to work 😉

@Godin

Godin approved these changes Aug 10, 2017

@Godin Godin merged commit bb2ecb5 into jacoco:master Aug 10, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Aug 10, 2017

Member

@chonton merged. Thank you for your contribution! 👍

Member

Godin commented Aug 10, 2017

@chonton merged. Thank you for your contribution! 👍

@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.