[build] compileKotlin task gets invalidated between builds

[grigoryk]

This causes unnecessary, (nearly?) full rebuilds. E.g. sometimes running an assembleDebug task, and then running it a few minutes later without any changes will perform a lengthy rebuild.

Same issue seems to affects tests - re-running unit tests in Fenix (without any code changes) seems to actually re-run them.

The problem appears intermittent - it doesn't always happen, but happens enough to be fairly easily noticeable locally. Also, doesn't appear new.

┆Issue is synchronized with this Jira Task

[grigoryk]

One scenario when this happens:

• run assembleDebug
• run some unit tests
• run assembleDebug again

You'd expect the second assembleDebug run to do nothing, but it sees that compileDebugKotlin task was not up-to-date, for the following reason:

Input property 'org.jetbrains.kotlin.allopen.annotation' has been removed for task ':app:compileDebugKotlin'

I think that's happening due to how we configure the allopen plugin in https://github.com/mozilla-mobile/fenix/blob/main/app/build.gradle#L244-L250 - looks like it's only applied if we're running a task that contains test or Test in its name.

Removing this condition and always applying this plugin solves this particular problem. Vice versa, re-adding the condition after a clean assembleDebug run, and it's again recompiling the world.

So, why was this plugin only enabled for test tasks, and does it matter? allopen will turn specially annotated final classes into open allow their extension. Which we then rely on in some of our tests - e.g. look at TestComponents extending normally final class Components.

I guess having this allopen plugin enabled for all tasks will allow us to create non-test classes that extend non-final classes that are annotated with Mockable. I think that's unlikely to happen.

Also, I don't think we really need this allopen thing, really. We can:

1. simply mark classes we currently extend in tests as open (functionally equivalent to enabling allopen for all tasks)
2. add interfaces for these classes that we can impl in tests
3. reconsider why we're even extending something as high level as Components with a bunch of mocks, just so that we can use it in FenixRobolectricTestApplication, for the sake of an absolutely giant bag of Android API mocks that is robolectric... None of these things sound to me like healthy unit testing practices.

For now, I think the simplest thing is to remove the plugin entirely, mark a few classes as open for the sake of existing tests, and then work through removing/rewriting tests that require this (e.g. can we stop using robolectric?).

This will remove compiler plugin magic, simplify things a bit and speed up our local builds.

Kind of off-topic, but for some reason I don't quite understand there's a bit of an indirection in how we have these annotation classes setup - in https://github.com/mozilla-mobile/fenix/blob/main/app/src/main/java/org/mozilla/fenix/utils/OpenClass.kt we have two annotations defined, OpenClass (which is what allopen is configured against, and separately Mockable (itself annotated with OpenClass) which is what's actually used in the codebase to mark classes that should become open.

Also, note that comments in OpenClass.kt are inaccurate - they talk about allowing extensibility in debug builds and not in release ones, which isn't true (and I don't know why you'd want that, tbh).

[grigoryk]

Fixing this allopen thing may speed up our CI as well, if we ever have a case where a compile task runs after some tests have run. @pocmo thinks we may have something like that happen in the (terribly slow) toolchain task, so 🤞

[grigoryk]

This was addressed in #22601

[grigoryk]

To close the loop here - I ended up removing allopen plugin, @Mockable and @Open annotations, and the TestComponents and TestCore classes that were used in tests. Bunch of unit tests that relied on the mocks provided by these global classes were updated to explicitly declare what mocks they actually needed.

The only class I ended up marking as open was StrictModeManager, due to existence of a TestStrictModeManager that wasn't as mechanical/trivial to remove.