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.

Sign up for GitHub

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

Jump to bottom

migrate test utils to use Test Fixtures plugin #2962

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

migrate test utils to use Test Fixtures plugin #2962

wants to merge 4 commits into from

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Apr 17, 2023 •
edited

Migrate test-util project to use Test Fixtures, to improve project organisation and structure.

This is a speculative PR that I have made because I think it a demonstration is more clear than trying to write it up in an issue. It introduces some breaking changes for 3rd party consumers, which might be undesirable.

Part of #2703

Summary

This PR migrates the existing test-utility projects to instead be contained inside the project in an independent source set, thanks to the Java Test Fixtures plugin.

  • code and dependencies for the test-util subprojects (:core:test-api, :core:content-matcher-test-utils, :plugins:base:base-test-utils) have been merged into their 'parent' subprojects
  • Local and remote projects do not need to apply the Test Fixtures plugin, and can depend on the test util code using the testFixtures() helper function embedded into Gradle.

Breaking changes

There are two breaking changes

Updated dependency coordinates for the test fixtures.

Currently the test utility projects are published as Maven artifacts (e.g. dokka-base-test-utils). These artifacts will no longer exist. Instead, the artifacts that contain the test fixtures (e.g. dokka-base) will have two variants published, which can be depended on as follows:

dependencies {
  testImplementation("org.jetbrains.dokka:dokka-base-test-utils:1.8.10") // old

  testImplementation(testFixtures("org.jetbrains.dokka:dokka-base:1.8.10")) // new
}

There are two potential workarounds to make this change a little less grating:

  • Keep the :plugins:base:base-test-utils projects (and others), which would expose the test fixtures as an API dependency, but would not contain any source code

    // plugins/base/base-test-utils/build.gradle.kts

dependencies {
  api(testFixtures(projects.plugins.base))
}

  • publish Maven relocation information

  • TODO verify that Maven coordinates for test fixtures are able to be used, and have separate coordinates

Binary Compatibility Validator support

BCV does not currently support the Java Test Fixtures plugin Kotlin/binary-compatibility-validator#117

I've created a fork of BCV that does support them https://github.com/adamko-dev/kotlin-binary-compatibility-validator-mu

@aSemy aSemy marked this pull request as draft April 17, 2023 13:13
aSemy
aSemy commented Apr 17, 2023
View reviewed changes
build-logic/build.gradle.kts
@@ -4,14 +4,15 @@ plugins {

kotlin {
jvmToolchain {
languageVersion.set(JavaLanguageVersion.of(8))
languageVersion.set(JavaLanguageVersion.of(11))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bcv-mu requires Java 11. Perhaps there's a better way around this, but it shouldn't matter. This line of config might not even be necessary any more.

build.gradle.kts Outdated
Comment on lines 39 to 53
apiValidation {
// note that subprojects are ignored by their name, not their path https://github.com/Kotlin/binary-compatibility-validator/issues/16
ignoredProjects += setOf(
// NAME PATH
"search-component", // :plugins:search-component
"frontend", // :plugins:base:frontend

"kotlin-analysis", // :kotlin-analysis
"compiler-dependency", // :kotlin-analysis:compiler-dependency
"intellij-dependency", // :kotlin-analysis:intellij-dependency

"integration-tests", // :integration-tests
"gradle", // :integration-tests:gradle
"cli", // :integration-tests:cli
"maven", // integration-tests:maven
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bcv-mu is applied on individual subprojects, so this configuration can be removed.

core/api/testFixtures/core.api Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file is the merged result of the :core:test-api and :core:content-matcher-test-utils projects' api dumps

core/content-matcher-test-utils/api/content-matcher-test-utils.api Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file was merged into core/api/testFixtures/core.api

kotlin-analysis/build.gradle.kts
Comment on lines +19 to +21
binaryCompatibilityValidator {
enabled.set(false)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the kotlin-analysis projects have API verification disabled, but perhaps they should indeed be validated? After all, they are published artifacts.

aSemy
aSemy commented Apr 17, 2023
View reviewed changes
build-logic/src/main/kotlin/org/jetbrains/conventions/maven-publish.gradle.kts
plugins {
id("org.jetbrains.conventions.base")
`maven-publish`
signing
id("org.jetbrains.conventions.dokka")
id("dev.adamko.kotlin.binary-compatibility-validator")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than BCV-classic's approach of validating all projects by default, and opting out on a case-by-case basis, I thought it made sense to apply BCV-MU in a slightly more targeted way and apply it to all published subprojects.

@aSemy aSemy marked this pull request as ready for review April 18, 2023 17:25
@aSemy aSemy force-pushed the feat/migrate_test_utils_to_test_fixtures branch from 4985014 to 3fc3dc8 Compare April 19, 2023 15:57
@IgnatBeresnev IgnatBeresnev self-requested a review April 25, 2023 20:31
@aSemy aSemy force-pushed the feat/migrate_test_utils_to_test_fixtures branch from 3fc3dc8 to 34f47d4 Compare June 3, 2023 15:41
@whyoleg
Copy link
Contributor

whyoleg commented Apr 8, 2024

Hey, @adam-enko, is it possible to finish this PR? (or reopen new one, as there were a lot of changes in the meanwhile)

@adam-enko
Copy link
Contributor

I think it's probably best to just close this PR. It's not entirely necessary to use test-fixtures, and if they would help, then it would make sense to start again from scratch with the latest changes from master.

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

@IgnatBeresnev IgnatBeresnev Awaiting requested review from IgnatBeresnev

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@aSemy @whyoleg @adam-enko