How does Kotest discover tests? Does it scan entir...

# kotest

[wasyl]

How does Kotest discover tests? Does it scan entire classpath or just the classes from the tests source set? Is it configurable somehow, if we want to only scan classes under specific package?

[sam]

Depends where you are invoking the tests from

[sam]

If using junit5 platform in gradle, gradle will pass the discovered tests through (it looks in the test source set)

[sam]

If using the intellij plugin, it will use the classname / package name directly

[wasyl]

(Context: after upgrading Android Gradle Plugin something has changed with how it handles Databinding classes. As a result, Kotest discover engine seems to trigger static initialisation of some of those Databinding classes, and that fails because static initialisers there use Android Framework dependencies 😕)

[wasyl]

If using junit5 platform in gradle, gradle will pass the discovered tests through

You mean tests or test classes? And Kotest will still filter those and only pick classes that extend from a Spec?

[sam]

It depends on the version/setup but gradle is supposed to pass in a discovery request, which includes things like classpath / package name, and then kotest scans those packages

[wasyl]

And that’s discover method in KotestJunitPlatformTestEngine, right?

[sam]

which version of kotest ?

[wasyl]

Oh, I see sources of runner-jvm-4.2.5 🤔

[sam]

There's kotest-framework-discovery module which contains the code

[sam]

the discovery method in

KotestJunitPlatformTestEngine

uses that depending on what gradle / intellij / whatever passes in

[sam]

If it's asking for a specific class, there's no need to run discovery

[sam]

There might be something in here: https://github.com/kotest/kotest/blob/master/kotest-framework/kotest-framework-discovery/src/jvmMain/kotlin/io/kotest/framework/discovery/Discovery.kt#L50-L74

[sam]

That is triggering the inits

[wasyl]

But if we run tests from entire module, ./gradlew foo:test the request will scan the classpath, right?

[sam]

yes

[sam]

It's not supposed to actually load the classes though, but perhaps that's a bug and it is

[wasyl]

yep, I discovered the doDiscover earlier 🙂 Just wanted to confirm that’s a good direction

[wasyl]

It actually might be a bug, because what we’re observing is

public class DataBinderMapperImpl extends DataBinderMapper {
  private static final SparseIntArray INTERNAL_LAYOUT_ID_LOOKUP = new SparseIntArray(0);

  private static class InnerBrLookup {
    static final SparseArray<String> sKeys = new SparseArray<String>(22);
  }
}

The offending line is the InnerBrLookup, not the outer one. I’m not sure if it’s because of the order of how the classes are loaded or it’s something about the class being an inner one

[wasyl]

https://github.com/mateuszkwiecinski/databinding_doesnt_work/tree/fixme_pls here’s an almost repro: it doesn’t depend on Kotest (was created before we figured out the exact problem) but if you replace vintage runner with Kotest, in library module, it’ll start failing on tests discovery

[sam]

Let me add a test case

[sam]

class Foo {
   companion object {
      init {
         error("boom")
      }
   }

}

[sam]

That should be the same right

[sam]

That causes the exception to throw when running the tests. Is your DataBinderMapperImpl in main or test.

[wasyl]

I believe it’s in both, although I don’t fully understand what AGP does with the classpath there

[wasyl]

But I’m gonna say it’s in tests because part of AGP 4.1 migration is running kaptTest so the classes would have to be available in tests now

[sam]

So if I have this class, Foo, that's never referenced from any test, it shouldn't be loaded by the classloader (in theory) and the init block should never fire

[sam]

At least if my understanding of the JVM is correct

[wasyl]

Yes, that’s my understanding as well

[wasyl]

And my understanding after some digging (but I’m not 100% sure) is that Kotest does load these classes somehow anyway, the nested one at least anyway

[wasyl]

Okay that thing you pasted isn’t the same, as there’s no static { } block in decompiled bytecode in the nested class, and the error is in the companion constructor. Not sure if it should technically make a difference, just pointing out it’s different

[sam]

Ok fair enough, I can try putting the java class in

[sam]

or use a @JvmStatic on a field that throws

[wasyl]

class Foo {
    class Bar {
        companion object {

            private val x: Nothing = error("boom")
        }
    }
}

[wasyl]

This is more similar, as there’s now static block that throws, in the inner class

[sam]

ok. Something odd is going on, because it's being invoked before kotest's discovery

[wasyl]

Not happy because it’s happening, happy because it’s a bug and we have a repro, right?

[sam]

yes

[sam]

once you have the repro, the rest is easy 🙂

[sam]

ok so here's the issue

[sam]

Gradle is sending over a list of classnames, and kotest is then doing Class.forName on them to find out if they're specs or not

[sam]

and of course, Class.forName will trigger the static inits

[sam]

so how do we find out if a class is a Spec without using Class.forName

[wasyl]

There’s also forName(String name, boolean initialize, ClassLoader loader), which as I understand shouldn’t trigger class initialization if initialize = false. However I don’t know if it changes something in the resulting class

[sam]

wow I've been doing reflection stuff since 1999 and never seen that method before

[sam]

and it says it was added in 1.2

[sam]

Ok that method seems to help

[sam]

I can use that first to filter down to specs and then run the inits after

[wasyl]

Is it required to manually init classes?

[sam]

To be safe I'm doing this

[sam]

// first filter down to spec instances only, then load the full class
val loadedClasses = request
   .selectors
   .filterIsInstance<DiscoverySelector.ClassDiscoverySelector>()
   .map { Class.forName(it.className, false, this::class.java.classLoader) }
   .filter(isSpecSubclass)
   .map { Class.forName(it.name).kotlin }
   .filterIsInstance<KClass<out Spec>>()

[wasyl]

I don’t understand what’s happening exactly, I just assumed JVM would initialize the classes whenever needed anyway

[sam]

wanna code review https://github.com/kotest/kotest/pull/1788

[sam]

I'm doing the 2nd class.forname because I don't know if the first time, by passing in false, it's going to mess with the class structure. Just to be safe, since we know it's a spec, I load it again the normal way.

[wasyl]

Right, they’d have to be loaded at some point anyway 👍

[wasyl]

PR looks good, just seems to have unintended enabled() change 🙂

[wasyl]

I can check if a snapshot works as expected

[sam]

oops reverted that enabled change, thanks

[sam]

yeah check the snapshot once I merge. And if good, I'll release 4.2.7 and 4.3.1 with the fix.

[sam]

I'm surprised no one has ran into this issue before

[wasyl]

I still don’t understand what Android Gradle Plugin was doing that this issue wasn’t triggered before 😕 Also I wonder if there’ll be any performance improvement from not initializing all the classes

[sam]

Yeah it shouldn't make it slower at least 🙂

[sam]

4.3.0.745-SNAPSHOT once it's built https://github.com/kotest/kotest/actions/runs/316419839

[wasyl]

@sam confirmed the snapshot works and fixes the issue in all our tests 🙂

[sam]

awesome 🙂

[sam]

Will release 4.3.1 and 4.2.7 soon

[wasyl]

Thank you very much. I recall the responsiveness to issues was why I chose Kotest over Spek back ~2 years ago and I’m glad to see that was the right choice 🙂

[sam]

Thanks, that's very nice to hear 🙂

[sam]

We wouldn't be able to fix issues as quickly if it wasn't for users like you who help find and test

[wasyl]

[Peter]

nice find & fix guys

[christophsturm]

maybe you could try to not load it a second time. if tests run multithreaded it could improve performance because its good to keep the single threaded init part as short as possible.