Update test-k2 convention to use test suites #3524

adam-enko · 2024-03-08T17:30:26Z

Summary

use JVM Test Suites to create create separate targets for descriptor and symbols tests
update Configurations to use newer Gradle guidelines for declarable/resolvable configurations

Usage

./gradlew test runs descriptor-compatible tests.
(Additionally, test tasks will trigger testDescriptors tasks.)
./gradlew testDescriptors runs descriptor-compatible tests (this is an alias of test, because it's easier to read).
./gradlew testSymbols runs symbols-compatible tests.

Benefits

Using JVM Test Suites unifies common test configuration for each of the test/descriptorsTest/symbolsTest tasks, so custom tasks aren't required
Avoiding custom tasks will help with Configuration Cache compliance.
Tagged tests are run in separate tasks, so the same tests aren't run multiple times with different tasks.
Remove deprecated behaviour - see Build Scan warning "Relying on the convention for Test.classpath in custom Test tasks has been deprecated."

- use jvm-test-suites to create create separate targets for descriptor and symbols tests - update Configurations to use newer Gradle guidelines for declarable/resolvable configurations

adam-enko · 2024-03-11T13:30:57Z

Hi @vmishenev 👋, I've added you as a reviewer since you created this convention plugin.

dkrasnoff · 2024-03-12T16:28:15Z

build-logic/src/main/kotlin/dokkabuild.test-k2.gradle.kts

+                    useJUnitPlatform {
+                        excludeTags.addAll(descriptorTags + symbolsTags)
+                    }
+                    classpath += descriptorsTestImplementationResolver.incoming.files


It is not clear to me why we have the same configuration as for descriptorsTest ?

Good question. Basically, Dokka needs to analyse source code.

For K1 source code, it uses a descriptor-based implementation.

And for K2, symbols-based implementation.

The Dokka subprojects don't use these implementations directly, they need to be provided at runtime. So, to run the tests, Dokka needs one implementation or the other. Dokka is still being converted to K2, so at the moment K1 is the default. Hence, the K1 descriptor-based analyser is used for 'regular' unit tests.

Does that help?

Correct me if I'm wrong please Dokka team!

That makes sense, thanks!

So, overall the idea is to run ALL tests with both K1 and K2 analysis. + there are some tests where behaviour differs (because f.e K1 or K2 analysis has some bugs), so for this cases we have annotations like OnlyDescriptors/OnlySymbols

And AFAIU, approach in this PR is not what we want. If im not wrong now we have 3 test tasks:

test will run ALL tests excluding marked with annotation using K1

descriptorsTest will run only marked for descriptors tests with K1

symbolsTest will run only marked for symbols tests with K2

So I believe it's not what we want.
I don't know how flexible are test-suites but finally we want to have two separate tasks to run analysis for K1 and K2, f.e:

descriptorsTest will run ALL tests excluding OnlySymbols with K1 runtime

symbolsTest will run ALL tests excluding OnlyDescriptors with K2 runtime

test will just run both symbolsTest and descriptorsTest, but don't run ALL tests one more time

Does it help?

symbolsTest will run ALL tests excluding OnlySymbols with K1 runtime
descriptorsTest will run ALL tests excluding OnlyDescriptors with K2 runtime

it seems @whyoleg mixed up symbolsTest and descriptorsTest
The correct order is

descriptorsTest will run ALL tests excluding OnlySymbols with K1 runtime

symbolsTest will run ALL tests excluding OnlyDescriptors with K2 runtime

So, on master we have:

descriptorsTest - runs tests with K1

symbolsTest - runs test with K2

test - configured the same as descriptorsTest, so runs tests with K1 + runs symbolsTest - so when running test you will be able to run tests both with K1 and K2

I see that descriptorsTest isn't added to check or test, so it won't always run

But, you can run it from IDEA gutter - so f.e I can run tests for some class/method with K1 only, or with K2 only

Ideally, I think, it will be fine that test will not run any tests by itself, but just run dependent tasks (for K1 and K2), but I'm not sure, that it's possible to make compatible with both CLI and IDE, so current approach (from master) should be a good compromise

Does it help to understand the idea better? May be Vadim has something additional to say here

Thanks for explaining! So the idea is to provide a more descriptive name, even if test and descriptorsTest are both the same, under the hood?

Ideally, I think, it will be fine that test will not run any tests by itself, but just run dependent tasks (for K1 and K2), but I'm not sure, that it's possible to make compatible with both CLI and IDE, so current approach (from master) should be a good compromise

Yeah, that makes sense. It's what I tried to do in the original PR, but JUnit and/or Gradle doesn't like having a test tasks that don't do anything (which is very important, because if a bug means no tests run then you don't want the test task to succeed!)

Anyway, I've reverted the behaviour to master, so it should work the same.

And here's a summary of the discussed options:

Task uses master initial PR test-runs-nothing proposal test-runs-no-tags proposal

test descriptors !K2 !K1 && !K2 nothing !K1 && !K2

descriptorTest descriptors !K2 K1 !K2 !K2

symbolTest symbols !K1 K2 !K1 !K1

may be we can then add comment the reason why test task is configured that way? So it's just an alias to descriptor tests, to avoid future confusions?

aside from comment, it would be good to actualise PR description

I have a question. Will the test task run the symbols tests as well?

Our CI (TeamCity/GH Actions) is set to run only the test task, e.g.

dokka/.github/workflows/tests-smoke.yml

Line 35 in ed3ab96

./gradlew test --stacktrace --no-daemon --no-parallel

# Conflicts: # build-logic/settings.gradle.kts

whyoleg · 2024-03-13T16:23:02Z

build-logic/src/main/kotlin/dokkabuild.test-k2.gradle.kts

+                    useJUnitPlatform {
+                        excludeTags.addAll(descriptorTags + symbolsTags)
+                    }
+                    classpath += descriptorsTestImplementationResolver.incoming.files


So, overall the idea is to run ALL tests with both K1 and K2 analysis. + there are some tests where behaviour differs (because f.e K1 or K2 analysis has some bugs), so for this cases we have annotations like OnlyDescriptors/OnlySymbols

And AFAIU, approach in this PR is not what we want. If im not wrong now we have 3 test tasks:

test will run ALL tests excluding marked with annotation using K1

descriptorsTest will run only marked for descriptors tests with K1

symbolsTest will run only marked for symbols tests with K2

So I believe it's not what we want.
I don't know how flexible are test-suites but finally we want to have two separate tasks to run analysis for K1 and K2, f.e:

descriptorsTest will run ALL tests excluding OnlySymbols with K1 runtime

symbolsTest will run ALL tests excluding OnlyDescriptors with K2 runtime

test will just run both symbolsTest and descriptorsTest, but don't run ALL tests one more time

Does it help?

- add comments to clarify K1 vs K2 analysis

build-logic/src/main/kotlin/dokkabuild/utils/gradle.kt

build-logic/src/main/kotlin/dokkabuild.test-k2.gradle.kts

whyoleg · 2024-03-19T10:32:59Z

build-logic/src/main/kotlin/dokkabuild.test-k2.gradle.kts

+                    useJUnitPlatform {
+                        excludeTags.addAll(descriptorTags + symbolsTags)
+                    }
+                    classpath += descriptorsTestImplementationResolver.incoming.files


may be we can then add comment the reason why test task is configured that way? So it's just an alias to descriptor tests, to avoid future confusions?

Update test-k2 convention to use test suites

e4d22bd

- use jvm-test-suites to create create separate targets for descriptor and symbols tests - update Configurations to use newer Gradle guidelines for declarable/resolvable configurations

adam-enko added the infrastructure Everything related to builds tools, CI configurations and project tooling label Mar 8, 2024

formatting

c65f374

adam-enko mentioned this pull request Mar 11, 2024

add explicit dependency on JUnit Platform Launcher #3525

Closed

adam-enko added 3 commits March 11, 2024 11:21

re-implement descriptorsTest

11764eb

update include/exclude tags

b53a62b

add comments, and always run symbol and descriptor tests

6208e9b

adam-enko self-assigned this Mar 11, 2024

tidy comments

11dde8f

adam-enko requested review from dkrasnoff, vmishenev and whyoleg March 11, 2024 13:29

adam-enko marked this pull request as ready for review March 11, 2024 13:30

dkrasnoff reviewed Mar 12, 2024

View reviewed changes

Merge branch 'master' into build-logic/update-test-k2

74b0521

# Conflicts: # build-logic/settings.gradle.kts

dkrasnoff approved these changes Mar 13, 2024

View reviewed changes

whyoleg requested changes Mar 13, 2024

View reviewed changes

adam-enko added 2 commits March 15, 2024 11:19

Merge branch 'master' into build-logic/update-test-k2

922c90d

- update custom targets to match current test behaviour on master

5a94613

- add comments to clarify K1 vs K2 analysis

adam-enko requested a review from whyoleg March 15, 2024 12:37

whyoleg approved these changes Mar 19, 2024

View reviewed changes

adam-enko added 3 commits March 20, 2024 15:30

rename descriptorsTest/symbolsTest tasks, update comments.

fb25fb0

update task links

9fd7391

update task links

2c7a7b3

vmishenev approved these changes Mar 20, 2024

View reviewed changes

adam-enko merged commit 1ed0186 into master Mar 22, 2024
12 checks passed

adam-enko deleted the build-logic/update-test-k2 branch March 22, 2024 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update test-k2 convention to use test suites #3524

Update test-k2 convention to use test suites #3524

adam-enko commented Mar 8, 2024 •

edited

adam-enko commented Mar 11, 2024

dkrasnoff Mar 12, 2024

adam-enko Mar 12, 2024 •

edited

dkrasnoff Mar 13, 2024

whyoleg Mar 13, 2024 •

edited

vmishenev Mar 13, 2024

whyoleg Mar 15, 2024

adam-enko Mar 15, 2024

whyoleg Mar 19, 2024

whyoleg Mar 19, 2024

vmishenev Mar 20, 2024

whyoleg Mar 13, 2024 •

edited

whyoleg Mar 19, 2024

Task	uses	master	initial PR	test-runs-nothing proposal	test-runs-no-tags proposal
test	descriptors	!K2	!K1 && !K2	nothing	!K1 && !K2
descriptorTest	descriptors	!K2	K1	!K2	!K2
symbolTest	symbols	!K1	K2	!K1	!K1

Update test-k2 convention to use test suites #3524

Update test-k2 convention to use test suites #3524

Conversation

adam-enko commented Mar 8, 2024 • edited

Summary

Usage

Benefits

adam-enko commented Mar 11, 2024

Choose a reason for hiding this comment

adam-enko Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyoleg Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyoleg Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-enko commented Mar 8, 2024 •

edited

adam-enko Mar 12, 2024 •

edited

whyoleg Mar 13, 2024 •

edited

whyoleg Mar 13, 2024 •

edited