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

Update Maven Integration Tests to use JVM Test Suites #3581

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

Conversation

adam-enko
Copy link
Contributor

@adam-enko adam-enko commented Apr 23, 2024

KT-64200

  • Updates dokka-integration-tests/maven to use jvm-test-suites

  • De-duplicates integration-test configuration into the dokkabuild.test-integration.gradle.kts convention plugin.

    • Note that the cli integration test suite was renamed from integrationTest to cliIntegrationTest to avoid clashing with the integrationTest lifecycle task.
  • Implement a new utility for passing in system properties to test tasks, and correctly registering them as Gradle inputs.

    Using System Properties is preferred to environment variables, because they are easier to register as Gradle task inputs.

- Migrate the CLI integration tests away from the custom integration test convention (which are not build-cache compatible) to use JVM Test Suites.
- Remove shadowing, replace with passing in the JARs via system properties. (a custom CommandLineArgumentProvider is confusing and overly complicated, but it's required to satisfy Gradle input normalization, so that the build-cache can be re-used.)
- Simplify `processUtils.kt` - the coroutines logic isn't necessary since the entrypoint just used `runBlocking {}`.
- move `jsonBuilder.kt` test-util into `src/main`, since it's not a test
- Rename `CliIntegrationTest` to `CliTest` (it wasn't an integration test, and the name was confusing compared to the actual `CliIntegrationTest`)
@adam-enko adam-enko force-pushed the KT-64200/maven-jvm-test-suite branch from 8b6dba6 to 16278e0 Compare April 24, 2024 12:50
@adam-enko adam-enko changed the title Update CLI Integration Tests to use JVM Test Suites Update Maven Integration Tests to use JVM Test Suites Apr 24, 2024
@adam-enko adam-enko force-pushed the KT-64200/maven-jvm-test-suite branch from e4cddd5 to 5a63254 Compare May 29, 2024 10:45
@adam-enko adam-enko marked this pull request as ready for review June 5, 2024 12:12
Refactor `fun MutableList<CommandLineArgumentProvider>.systemProperty()` to be more consistent with the other systemProperty utils, and so the intention is clearer.
@adam-enko adam-enko force-pushed the KT-64200/maven-jvm-test-suite branch from 0939e5d to b887dc5 Compare June 18, 2024 14:18
Copy link
Member

@ALikhachev ALikhachev left a comment

Choose a reason for hiding this comment

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

LGTM

a side note: would be nice to move test projects from the project level directory projects to the resources of the related test suit source set

Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

just some minor things after latest commits :)

* because such a property might disrupt task caching.)
*
* If you want to register the property as a Task input, use the
* [Test.systemProperty][dokkabuild.utils.systemProperty] above instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be rephrased now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, how do you think it could be rephrased?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, it's now a function inside an object which is created by [Test.systemProperty], so may be suggesting just using other property will be more appropriate here, like:

Suggested change
* [Test.systemProperty][dokkabuild.utils.systemProperty] above instead.
* [inputProperty] above instead.

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.

None yet

3 participants