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

Maven goal report-aggregate should consider dependencies specified using version range #658

Merged
merged 10 commits into from
Aug 17, 2018

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Mar 15, 2018

Fixes #657.

@metlos metlos changed the title Dependencies specified using a version range are considered for the aggregate report Dependencies specified using a version range aren't considered for the aggregate report Mar 15, 2018
@marchof marchof requested a review from Godin May 8, 2018 18:46
@marchof
Copy link
Member

marchof commented May 8, 2018

@Godin What do you think about this PR? Beside adding a test this looks good to me.

@metlos
Copy link
Contributor Author

metlos commented Jul 13, 2018

I've added an integration test heavily based on the it-report-aggregate. It should check that the jacoco files are loaded from locations of the projects in reactor that conform to the version range.

@Godin
Copy link
Member

Godin commented Jul 13, 2018

@marchof to be honest every implementation of aggregate reports for Maven look like workaround/hack for inflexible lifecycle of Maven build. And so I don't like them in general as a source of user confusions, misconfigurations, etc. This change looks like a continuation of workarounds/hacking. Nevertheless couple of more constructive comments here and inline in changes:

@metlos

I've added an integration test heavily based on the it-report-aggregate

I'm wondering why instead not simply update/extend it-report-aggregate? The reason of this question is that each integration test is very costly in terms of execution time, existing ones are already slowest part of our build and much slower than any other part. In Travis for latest build of this PR with JDK 8: 16.9 s for it-report-aggregate-version-ranges, 13.0 s for it-report-aggregate. Maybe will be even better to get rid / combine it-report-aggregate-customization, which was 9.5 s.

depVersionAsRange = VersionRange.createFromVersionSpec(d.getVersion());
} catch (InvalidVersionSpecificationException e) {
throw new IllegalStateException("Could not parse the version of the dependency even though Maven" +
" could: " + d);
Copy link
Member

Choose a reason for hiding this comment

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

Can this ever happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should not, really, but InvalidVersionSpecificationException is a checked exception, so there has to be something done about it...

Copy link
Member

@Godin Godin Jul 13, 2018

Choose a reason for hiding this comment

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

If I'm not mistaken, then we use throw new AssertionError(e) for such things - e.g.

@metlos
Copy link
Contributor Author

metlos commented Jul 13, 2018

I'm wondering why instead not simply update/extend it-report-aggregate?

Well, I've seen another version of the report aggreate it-test - it-report-aggregate-customization and so I thought this was the approach you take. I can merge it into the main it-test if you wish..

@Godin Godin self-assigned this Jul 14, 2018
@Godin Godin added this to the 0.8.2 milestone Aug 12, 2018
@Godin Godin added this to Review in Current work items via automation Aug 14, 2018
@Godin Godin changed the title Dependencies specified using a version range aren't considered for the aggregate report Maven goal report-aggregate should consider dependencies specified using version range Aug 16, 2018
@Godin
Copy link
Member

Godin commented Aug 16, 2018

For the record: during testing I stumbled upon a fact that when reactor contains two modules with same groupId:artifactId but of different versions (exactly like integration test), then first dependency which satisfies range will be picked. I.e. in case of range [0,2]

  • if version 1 is before version 2 in reactor, then version 1 will be picked
  • and vise versa - if version 2 is before version 1 in reactor, then version 2 will be picked

This looks acceptable to me - case seems rare, range can be narrowed/specified to select correct version from reactor.

@Godin Godin force-pushed the support-version-ranges-in-report-aggregate branch from 3d33f29 to dd7bcf8 Compare August 16, 2018 21:37
@Godin Godin force-pushed the support-version-ranges-in-report-aggregate branch from 667e426 to fb6c2e7 Compare August 16, 2018 21:48
@Godin Godin force-pushed the support-version-ranges-in-report-aggregate branch from 8671717 to 95f64e7 Compare August 16, 2018 22:11
@Godin Godin merged commit 63d55ac into jacoco:master Aug 17, 2018
Current work items automation moved this from Review to Done Aug 17, 2018
@Godin
Copy link
Member

Godin commented Aug 17, 2018

@metlos thank you for your contribution which is now recorded in changelog ❤️

@jacoco jacoco locked as resolved and limited conversation to collaborators Oct 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants