Skip to content

Conversation

rstata
Copy link
Collaborator

@rstata rstata commented Sep 20, 2025

Description

See Issue #30. Copying the description: "Many tests of the Tenant API implementation are performed by integration tests that depend on code generation. The source for these tests is found in integrationTest source sets, but are built an run by the integration-test Gradle projects (e.g., tenant/runtime-integration-tests). The off-the-shelf configuration of Jacaco does not include those tests in its coverage reports, so those reports are not as useful as they could be. The goal of this issue is to configure Jacoco so that it includes integration tests as testing code from their respective main source sets."

Motivation and Context

How has this been tested?

Manual creation and inspection of coverage reports.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ x] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • [ x] My code follows the code style of this project.
  • [x ] My change requires a change to the documentation.
  • [ x] I have updated the documentation accordingly.
  • [ x] I have added tests to cover my changes.
  • [x ] All new and existing tests passed.

@rstata rstata force-pushed the covagg branch 2 times, most recently from e80675f to 862d62c Compare September 22, 2025 05:44
* with the unit tests from the base project to generate a full coverage report
*/

import org.gradle.api.attributes.*
Copy link
Collaborator

@fireboy1919 fireboy1919 Sep 22, 2025

Choose a reason for hiding this comment

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

I'm sure you are aware of this, but this seems like a lot of gradle specific, quasi-internal details, and therefore might be fragile.

This seems like a common type of thing to do. Perhaps we can find an example on the internet of a codegen tool that has Jacoco integration tests that does this in a simpler way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm not sure i see that we're using quasi-internal details here. that said, i did take a pretty serious look at an alternative. in particular, the jacoco-report-aggregation plugin. i think there are things that i'm doing in the tasks created by registerCoverageTask that might be built into jacoco report aggregation. that said, any complexity here is due to the fact that the base project and the integration-test project are in different builds in our composite build. this means we should be using configurations to connect these tasks. in attempting a quick prototype that used jacoco report aggregation, i was quickly recreating pretty much everything in this PR.


includeBuild(".")
includeBuild("included-builds/core")
includeBuild(".") // TODO - is this needed?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's necessary. It allows the viaduct-bom project to be resolved by other included builds (like the demo apps).

@jbartok jbartok self-requested a review September 23, 2025 06:38
Copy link
Collaborator

@jbartok jbartok left a comment

Choose a reason for hiding this comment

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

See my comments, we can just address them in a later pass.

attribute(Category.CATEGORY_ATTRIBUTE, objects.named(Category.VERIFICATION))
attribute(Usage.USAGE_ATTRIBUTE, objects.named(Usage.JAVA_RUNTIME))
attribute(Bundling.BUNDLING_ATTRIBUTE, objects.named(Bundling.EXTERNAL))
attribute(LibraryElements.LIBRARY_ELEMENTS_ATTRIBUTE, objects.named("jacoco-exec"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the other attributes are necessary, this one is a critical one. We should try removing the other ones (here and also in other similar attribute sets).


// The following is not just a cross-project task-dependency, it's a cross-build one.
// However, configurations alone were not enough to force execution of the :sourcesJar task.
// TODO - fix this (maybe a proper task subclass would fix this).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Theoretically this should be as simple as:

tasks.register<Copy>("unpackRuntimeClasspathJars") {
    val outputDir = layout.buildDirectory.dir("unpacked-libs")
    from({
        configurations.runtimeClasspath.get().map { zipTree(it) }
    })
    into(outputDir)
}

If it's not, then maybe the problem is not here, but in the projects that generate the sources.

I would say, if it works, leave it and we can come back to it later, investigate, improve.

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