[proposal] Always reset all test fixtures

[mgulbronson]

Description

Currently if the injector is not reused no test fixtures are reset, presumably because it's assumed the test fixtures are scoped to the injector and will therefore be reconstructed for each test when the injector is created. But test fixtures that are Kotlin objects aren't scoped to the injector, they're scoped to the JVM classloader and therefore persist across tests/injectors. So if the injector is not reused, those test fixtures never get reset.

Practically this means that binding a test fixture doesn't do anything. This PR makes it so all test fixtures are always reset, making it possible to implement test fixtures as objects.

---

Alternative approach that I did originally was to reflect on the test fixtures to check if they're an object or not. For test fixtures that aren't objects no change from the current behaviour is needed. For test fixtures that are objects they need to be reset every time (regardless of whether the injector is being reused). But realistically the cost of reflecting over the test fixtures is more expensive than just resetting all test fixtures.

[adrw]

Makes sense, going to wait for one more review and then merge.

[mmollaverdi]

But test fixtures that are Kotlin objects aren't scoped to the injector, they're scoped to the JVM classloader and therefore persist across tests/injectors. So if the injector is not reused, those test fixtures never get reset.

I might be misunderstanding this, but testFixtures.reset() only reset what's scoped to the an injector, because testFixtures is an @Injected value. So the change in this PR won't change that behaviour.

Curious though, are you leveraging misk test injector reuse to speed up your tests?

[mgulbronson]

But test fixtures that are Kotlin objects aren't scoped to the injector, they're scoped to the JVM classloader and therefore persist across tests/injectors. So if the injector is not reused, those test fixtures never get reset.

I might be misunderstanding this, but testFixtures.reset() only reset what's scoped to the an injector, because testFixtures is an @Injected value. So the change in this PR won't change that behaviour.

@mmollaverdi I think if you use instance bindings then it'll have an effect. For instance if I have

object FakeFoo : Foo, TestFixture

multibind<TestFixtures>().toInstance(FakeFoo)

You're right that the binding itself is scoped to the injector, but the actual thing it's injecting (FakeFoo) is not. So in this case, injecting a Set<TestFixture> would inject the singleton Foo (which may have state from previous test runs). Before my change we wouldn't reset it unless we're re-using the injector, now we will. Let me know if that makes sense

Curious though, are you leveraging misk test injector reuse to speed up your tests?

We do not — I'm not sure exactly why as I wasn't around for our initial setup but IMO it's confusing regardless that object test fixtures don't properly get reset

[mmollaverdi]

Makes sense

[mgulbronson]

@adrw or @mmollaverdi either of you able to approve the workflow so we can merge this thing? much appreciated for your help with this PR! 🙏

[adrw]

@mgulbronson Apologies for the delay, I thought this had already merged, I'll try and shepherd it through today.

[adrw]

@mgulbronson Not sure why but some test seems to keep timing out in the testShardNonHibernate, I've triggered a rebuild a few times so it might not be a test flake but a regression from your code change. Do you mind taking a look?

[mgulbronson]

@mgulbronson Not sure why but some test seems to keep timing out in the testShardNonHibernate, I've triggered a rebuild a few times so it might not be a test flake but a regression from your code change. Do you mind taking a look?

@adrw I think I see the issue — will try to get a fix up today but may wait until after the holidays