dokka Do not run integration tests on `check`

Mar 05 '24 16:03 IgnatBeresnev

Whether integration tests should be run on check was discussed at length here: https://github.com/Kotlin/dokka/pull/3427#discussion_r1463181211

I needed to run build after updating KGP to 2.0.0 to see if anything fails locally, thinking "if it builds locally - I'll open a PR and it'll trigger integration tests", but I was unable to do so, because build triggers check, which triggers integration tests.

I found no way to run build without integration tests, and I don't want to wait for 2 hours or trigger build separately for all subprojects but dokka-integration-tests. It seems @atyrin was in a similar situation recently

If the majority of core developers agree that it's not convenient, I think it should be removed, even if it breaks some conventions. @whyoleg @vmishenev @atyrin could you please vote with 👍 or 👎 ? If the majority thinks the way it is now is convenient, we can leave it, no problem

To clarify,

without this change (PR rejected): ./gradlew check and ./gradlew build trigger integration tests, but ./gradlew test does NOT
with this change (PR accepted): only ./gradlew integrationTest triggers integration tests, while clean, build and test do not

Mar 05 '24 16:03 IgnatBeresnev

@whyoleg @vmishenev I see you've already approved the PR while I was writing the message, but just in case: this is not required (not a top-down decision), you can vote against it if you think that check should run integration tests. We'll go by majority

Mar 05 '24 16:03 IgnatBeresnev

Since we are here, could you adjust the contribution guide regarding the new way of running/excluding integration tests? And probably GitHub workflows (if it required)?

Mar 05 '24 16:03 atyrin

I needed to run build after updating KGP to 2.0.0 to see if anything fails locally, thinking "if it builds locally - I'll open a PR and it'll trigger integration tests", but I was unable to do so, because build triggers check, which triggers integration tests.

I found no way to run build without integration tests, and I don't want to wait for 2 hours or trigger build separately for all subprojects but dokka-integration-tests. It seems @atyrin was in a similar situation recently

'build' is a lifecycle task that runs both 'check' and 'assemble', so if you want to skip checks, can you just run `./gradlew assemble'?

Mar 08 '24 09:03 adam-enko

'build' is a lifecycle task that runs both 'check' and 'assemble', so if you want to skip checks, can you just run `./gradlew assemble'?

The idea is not to just check that we can assemble project (so build all artefacts), but to check that everything works after an update, so tun run tests. To run all tests we need to run check (or build), but now, if you will just run check, you will run integration tests - and they are really long and running them locally is not very convenient. Look on integration tests like an optional local step, but required CI step. Locally integration tests need to be run on a case by case bases, while all other tests are required.

So, another possible option: make check depends on integration tests only on CI via env variable - though, I don't see why do we need this, as on CI, we anyway run integration tests and all other tests in separate jobs - though, just removing this relation looks fine for me and it will be enough to just documented this in developer guide. If the user will need to run integration tests locally - they know why, they configured Android SDK, they have time to wait for result, and so on.

Mar 14 '24 10:03 whyoleg

'build' is a lifecycle task that runs both 'check' and 'assemble', so if you want to skip checks, can you just run `./gradlew assemble'?

Yeah, the situation is pretty much how Oleg described it. I updated something that may affect all parts of Dokka in various ways: maybe something won't compile or will result in a warning, maybe some unit tests won't pass, maybe we return a type from stdlib somewhere in our public API and it was changed incompatibly, and so on. So I want to run all tests and checks without running the integration tests (to catch all obvious problems), and then submit a PR (which runs integration tests) and switch to working on a different issue. This is quite a frequent scenario, at least for me

I've updated the documentation, both CONTRIBUTING.md and developer guides

Mar 14 '24 13:03 IgnatBeresnev

Here's a summary of the reasonable options, in tabular form. What I want to demonstrate is that I'm not sure how to achieve the tasks with ❓ in this PR, while it is possible to run quick checks in all scenarios.

	master	this PR	implement custom quickCheck	add `test.dependOn(apiCheck)`
I want to run all verifications	check	❓	check	check
I want to run all integration tests	:dokka-integration-tests:check	❓	:dokka-integration-tests:check	:dokka-integration-tests:check
I want to run quick verifications	test apiCheck	check	quickCheck (+ `quickCheck.dependsOn(test, apiCheck)`	test

I agree that it's rarely convenient to run all tests, and so it's important to have an alternative.

For some more context, we're ~4 PRs away from being able to enable Configuration Cache (for the Dokka build scripts). This will allow for parallel tasks, reducing the time to run all tests on my machine to ~20 minutes. Which is still too long for a quick check, but a significant improvement.

I've been working on the integration tests a lot so it impacts me if there's no easy way to run all checks. It also adds some friction when I switch between projects that don't follow the Gradle conventions.

Mar 15 '24 12:03 adam-enko

What I want to demonstrate is that I'm not sure how to achieve the tasks with ❓ in this PR, while it is possible to run quick checks in all scenarios.

I haven't checked, but from my understanding with this PR it will be:

I want to run all verifications - check integrationTest
I want to run all integration tests - integrationTest
I want to run quick verifications - check

As for me, It looks like a good balance, as running integrationTest is mostly for CI needs and occasionally for local needs.

Mar 19 '24 10:03 whyoleg

I want to run all verifications - check integrationTest

I want to run quick verifications - check

These two make sense, thanks.

I want to run all integration tests - integrationTest

This will miss some integration tests, e.g. https://github.com/Kotlin/dokka/blob/master/dokka-integration-tests/gradle/src/test/kotlin/StdLibDocumentationIntegrationTest.kt

Mar 19 '24 17:03 adam-enko

This will miss some integration tests, e.g. https://github.com/Kotlin/dokka/blob/master/dokka-integration-tests/gradle/src/test/kotlin/StdLibDocumentationIntegrationTest.kt

While this is possible to overcome, I see, that there are some quirks here and there and understanding of everything is far from ideal.

I still think, that what is proposed in this PR is a good balance for all of us and integration tests should be optional locally (even if they run for "just" 20 minutes).

Last idea that I had in mind, after it I'm done with arguing on this topic: Make root lifecycle tasks (check, build, test, etc) don't depend on dokka-integration-tests tasks at all. So that there will be clear separation of the main build with its included builds (subprojects + runners) and integration tests. So points from table will look like:

I want to run all verifications - check integrationCheck / build integrationBuild
I want to run all integration tests - integrationTest / integrationCheck
I want to run quick verifications - test / check

If needed we could introduce fullCheck/fullBuild/etc (or any other prefix/suffix). So the idea is to not introduce new task quickCheck which runs part of verifications but go from the other side.

The reasoning is the same and is simple: integration tests are not required for local day-to-day development. While Gradle has some conventions (who has not?), I think that we do need to not strictly follow conventions, but be idiomatic (your now what I mean, we are working with/on Kotlin). And IMO, idiomatic here - something that is used more frequently (running tests and basic checks like BCV) should be fast, easy, more understandable, convenient, familiar to users (Dokka Team and external contributors) and running integration tests is not one of such things, as it could take abnormal amount of time on not so new PC (not everyone has Mac M1/M2/M3 with 12 cores) and can even fail for unrelated reason because something is configured wrong locally (f.e Android SDK)

Mar 20 '24 12:03 whyoleg

Make root lifecycle tasks (check, build, test, etc) don't depend on dokka-integration-tests tasks at all.

Actually, this is a good idea to explore! @adam-enko would it make it any better in your opinion?

Run project checks that are stable and expected to be run locally: ./gradlew check
Run integration tests: ./gradlew integrationTest (which depends on dokka-integration-tests:check) - purely for the convenience of typing
Run checks with integration tests: ./gradlew check integrationTest

Then dokka-integration-tests:integrationTest task can be removed, so that you can run dokka-integration-tests:check and have all integration tests be executed (I'll revert check.dependsOn(suites))

It looks kind of like middle ground: dokka-integration-tests is independent and conventional on its own (check runs all integration tests), yet the root check task does not run integration tests for the reasons outlined above, which is the primary thing that we want

Mar 21 '24 13:03 IgnatBeresnev

Our colleagues from the Kotlin Analysis API team stumbled upon this recently - they wanted to build Dokka locally to test some K2-related fixes, but because the integration tests are run by default, they couldn't (due to problems with configuration/resources). Sadly, this is an instance of what was mentioned above, where it starts being an inconvenience if you're out of context and don't know how to deal with it

I'll merge this PR now to unblock colleagues, but I'd be happy to continue the discussion on how to implement in a better way here or someplace else

Apr 08 '24 11:04 IgnatBeresnev