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

Implement analysis test API #3184

Merged
merged 4 commits into from
Oct 27, 2023
Merged

Implement analysis test API #3184

merged 4 commits into from
Oct 27, 2023

Conversation

IgnatBeresnev
Copy link
Member

@IgnatBeresnev IgnatBeresnev commented Sep 21, 2023

Adds convenient API for testing Kotlin analysis implementations.

Comment on lines +16 to +25
/**
* Should NOT be used outside of Dokka itself, there are no guarantees
* this class will continue to exist in future releases.
*
* This class resides in core because it is a non-trivial implementation
* for a core extension [CoreExtensions.documentableMerger], which is needed
* in modules that only have access to `dokka-core`.
*/
@InternalDokkaApi
public class DefaultDocumentableMerger(context: DokkaContext) : DocumentableMerger {
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this class to core since I need it for bootstrapping analysis tests.

Don't really understand why we need the DocumentableMerger extension point at all - I find it very difficult to believe someone will need to re-implement it or will even understand how it works (I don't). Should return to it some time in the future when we revisit all core extensions.

@IgnatBeresnev IgnatBeresnev marked this pull request as ready for review October 5, 2023 12:53
@IgnatBeresnev IgnatBeresnev self-assigned this Oct 5, 2023
@IgnatBeresnev IgnatBeresnev added this to the Dokka 1.9.20 milestone Oct 12, 2023
moduleName = "java-module-name-for-unit-test"
}
javaFile(pathFromSrc = "org/jetbrains/dokka/test/java/Bar.java") {
+"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to remove the plus operator for strings?

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand it might lead to some issues down the road, but I couldn't think of any substantial problems to justify changing it as our use case is rather simple.

This API is not public, so we'll have a chance to change it quickly if it's ever published externally.

verifyFilePathStartsWithSlash("additionalSourceRoots", it)
}
additionalClasspath.forEach {
verifyFileExtension("additionalClasspath", it, ".jar") // should it include .klib?
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be a list of JARs, KLIBs, folders with .class files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix the comment 👍. Let's change the actual check once we have the first related tests and are sure that they work.

Copy link
Contributor

@vmishenev vmishenev Oct 18, 2023

Choose a reason for hiding this comment

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

Have you checked that it works with JVM stdlib: kotlin/jvm/Strictfp.class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry, missed the reply.

Not sure what you mean exactly. The only place where kotlin/jvm/Strictfp.class is used is to find the .jar file that contains it - passing the .jar file here works, yeah. But for now there's no need and there are no tests that pass individual class files

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad. I thought jvmStdlibPath was an unpacked folder.

But for now there's no need and there are no tests that pass individual class files

Indeed, the stdlib jar is only used in the classpath.

// this check can be extended to accept .klib, .class or other files

I'm not sure Dokka's analysis can work with individual .class files. But it should work with directories containing .class files, e.g. someProject/build/classes/.

import kotlin.test.Test
import kotlin.test.assertEquals

class JavaAnalysisTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Strict explicit API mode does not work here, in tests. It should have public

Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit API is deliberately disabled in tests, it only makes sense for main sources

/**
* The main logger used for running Dokka and analyzing projects.
*
* Changing the level to [LoggingLevel.DEBUG] can help with debugging faulty tests
Copy link
Contributor

Choose a reason for hiding this comment

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

How a user can change a logging level?

Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Oct 17, 2023

Choose a reason for hiding this comment

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

At the moment, the only user is us (it's not public), so I thought we would just be able to change it temporarily in the source code.

But in #3195 I needed to assert some logging messages, so this logger became the default logger, and you can just pass your own one to the parse function.

* as the input user project. This allows us to simulate Dokka's behavior and results
* on a made-up project as if it were real and run via the CLI runner.
*
* Executes only a limited number of steps and uses a small subset of [CoreExtensions]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it used only one extension point CoreExtensions.sourceToDocumentableTranslator?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I think yes. It would call the documentable merger extension point, but the only implementation is registered in dokka-base, so it's just created as a plain object for every invocation

*
* Works only with single-module projects, where the source code of this project
* resides in the root `src` directory. Works with multiple source sets and targets,
* so both simple Kotlin/JVM and more complicated Kotlin Multiplatform project must work.
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, it works only with Kotlin/JVM, not KMP?

Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Oct 17, 2023

Choose a reason for hiding this comment

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

At the moment there's API to test JVM projects only, but it doesn't mean it's not going to work for KMP projects :) Once KMP test project API is added - hopefully it'll just work, I don't see a reason why it shouldn't. Even if it doesn't - the KDoc clearly states that it must, so it will need to be adapted to support it

@IgnatBeresnev IgnatBeresnev merged commit edcd1fb into master Oct 27, 2023
9 checks passed
@adam-enko adam-enko deleted the analysis-test-api branch September 25, 2024 16:34
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.

3 participants