IssueTracker

I opened a PR to improve NonNullableMutableLiveDataDetector's type references check.
https://github.com/androidx/androidx/pull/161

Project: platform/frameworks/support
Branch: androidx-main

commit 655e08dca6e27aa15c862132914cb61b0ec29fdf
Author: Sinan Kozak <kozaxinan@gmail.com>
Date:   Wed May 05 16:38:12 2021

    [GH] [Lifecycle] [Lint] fixes b/184830263 Lint should use check reference too
   
    ## Proposed Changes
   
      - Fix NonNullableMutableLiveDataDetector for  [different cases](https://issuetracker.google.com/issues/184830263). It is possible to only set type reference and omit call's generic argument.
      -  Added check for generic values because default upper bound of generic (if none specified) is Any?.  If generic is not specified as `<T : Any>`, `T` can be defined null at call site. https://kotlinlang.org/docs/generics.html#upper-bounds
   
    ## Testing
   
    Test: Run NonNullableMutableLiveDataDetectorTest with new edge cases.
    `nullLiteralFailMultipleFields` : Added type reference
    `justKotlinObject` : Add additional test to prevent ArrayIndexOutOfBoundsException. It is fixed in https://github.com/androidx/androidx/commit/428f0ec685ff80035c0511c0cdb12b9770b6159c but not tested. It is reported here with [b/184830262](https://issuetracker.google.com/issues/184830262)
    `genericParameterDefinition` : Generics are assumed as nullable, lint should ignore.
   
    ## Issues Fixed
   
    Fixes: The bug on [b/184830263](https://issuetracker.google.com/issues/184830263) being fixed
    Fixes: Tests [b/184830262](https://issuetracker.google.com/issues/184830263)
   
    This is an imported pull request from https://github.com/androidx/androidx/pull/161.
   
    Resolves #161
    Github-Pr-Head-Sha: e96b4a8e860002feccd455e9ad8dcdd868dff2df
    GitOrigin-RevId: eb21c2ef19da5465be438c928d9b6ba2498b72e8
    Change-Id: Id6cd01e9ec6b79a3f14b569b53d79627c79b9866

M       lifecycle/lifecycle-livedata-core-ktx-lint/src/main/java/androidx/lifecycle/lint/NonNullableMutableLiveDataDetector.kt
M       lifecycle/lifecycle-livedata-core-ktx-lint/src/test/java/androidx/lifecycle/lint/NonNullableMutableLiveDataDetectorTest.kt

https://android-review.googlesource.com/1697465

??

Using a system property might be good as a workaround, but we'll probably keep using files as I think files are a cleaner way of passing information among tasks/plugins.

The challenge is that Robolectric seems to rely on the test config file containing absolute paths, so changing to relative paths immediately would break it.

To address this, I'm planning to introduce a temporary flag to enable the new behavior: When this flag is true, the test config file will contain relative paths, and Robolectric needs to update to work with relative paths. Once Robolectric is updated without issues, we will flip the flag to true by default and eventually remove the flag. We might enforce a minimum version of Robolectric at that point.


@Christian: FYI on this upcoming change in the AGP. Please let me know if you anticipate any issues with this. Thanks!

I think we should consider making that temporary flag true by default, unless Robolectric is detected. Would that be possible? Otherwise projects that don't use Robolectric but _do_ include resources would lose relocatability. Or is that just not a valid use-case?

I wanted to first clarify why we must implement this transition through a flag (cc'ing Chris):
    - We've publicly documented that the test config file contains absolute paths: https://google.github.io/android-gradle-dsl/current/com.android.build.gradle.internal.dsl.TestOptions.UnitTestOptions.html#com.android.build.gradle.internal.dsl.TestOptions.UnitTestOptions:includeAndroidResources. So if any users of this includeAndroidResources property rely on the file containing absolute paths, changing to relative paths would break them. Because we don't know our users, we have to assume the worst.
    - Even if Robolectric is the only user of this property, I've checked (via a test project using this change) that it would also break Robolectric. To be clear, Robolectric can already work with the relative paths of most properties, except for the android_resource_apk property (not documented at the above link, but it exists). As soon as I change this property to relative path, Robolectric will fail. I've tracked that it is because this line of code requires the property to be an absolute path: https://github.com/robolectric/robolectric/blob/56681651992c3aa5e1cc08744a08a235a8481954/robolectric/src/main/java/org/robolectric/android/internal/ParallelUniverse.java#L277

Now, regarding the default value of the flag, because of the above breakage (to Robolectric) and risk (to users other than Robolectric), I believe it should be false by default during at least the first few canaries of 3.5.

At the end of the canary phase or when we enter the beta phase, we'll probably re-consider this default value. It will depend on factors such as: whether Robolectric has been updated to work with this change, how many bug reports we have received, how many users still use the old version of Robolectric (and have issues updating), etc.

That makes a lot of sense. Thanks for explaining.

Thanks for the explanation - using a flag makes sense. How does Robolectric figure out the base directory to use for obtaining the absolute paths? Would it work when running tests either in the command line or within Android Studio?

The base directory used by robolectric is the working directory of the test executor, which is the project root by default.
Making this not brittle to changing of the test executor current working directory might be useful, but perhaps can wait.

FYI, I've filed https://github.com/robolectric/robolectric/issues/4571 for Robolectric to add support for relative paths.

Also, just to be clear - it is the project directory, not the root directory, right? Would this work even if the build directory is not located under the project directory? (As an example, all our build directories are until one single /build directory under the root project)

All the paths will be relative to the root project's directory. I have highlighted that in https://github.com/robolectric/robolectric/issues/4571 so Robolectric authors are aware.

Test executors, however, seem to use the project directory, not the root directory, as the working directory (based on my testing on both gradle and AS). How would Robolectric obtain the root project directory, in order to obtain the absolute paths?

I don't know how Robolectric handles paths internally. I've forwarded the question to https://github.com/robolectric/robolectric/issues/4571, and will update the AGP if necessary so it can work for all sides.

Sorry my earlier observations were incomplete. I've updated it at https://github.com/robolectric/robolectric/issues/4571:
    - If I use the root project directory as base, Robolectric will fail.
    - However, if I use the current project directory as base, then Robolectric 4.0+ already support relative paths.

I'm planning to change the AGP to use the current project directory as base now.

@César: Would that work for your use case? If not, could you clarify the issue on the Robolectric bug so we can figure out a way forward?

Yeah, thanks, that's what I meant when I said earlier that both Gradle and AS seemed to run tests using the project directory. Using paths that are relative to the project directory sounds perfect to me.

That sounds good.

Starting with AGP 3.5.0-alpha05, the following Gradle property will be available to use:
    android.testConfig.useRelativePath
It currently defaults to false. When set to true, the test config file will contain all paths relative to the current project directory. It should already work with Robolectric 4.0+.

Relevant commits:
    - https://android.googlesource.com/platform/tools/base/+/e1d267e226d7c69cebe8f53a4df519ab658a1a67
    - https://android.googlesource.com/platform/tools/base/+/e80be3495eab92186ef955850ba2ada7ff514caa


Please try out this feature and let us know if you see any issues. I'm expecting to set this flag to true by default soon. Leaving this bug open to keep track of that.

When do you expect 3.5.0-alpha05 to be published? The latest I can find right now is alpha03.

I think it will be released at the end of next week.

Hi,

Is there a plan to make this available even in future versions of AGP 3.4 ?

I'm afraid not, as we have gone from beta to RC for 3.4, meaning it's a lot harder to cherry-pick fixes.

Starting with AGP 3.6.0-alpha02, this flag android.testConfig.useRelativePath will be enabled by default. Android unit tests should be fully cacheable now. Thanks for the reports and discussion!

Change: https://android.googlesource.com/platform/tools/base/+/e48c3831b9a9dd4829d63ba80fddc46ba7e67e61

I'd like to point out a pitfall with unit test caching that should probably be fixed in gradle: test ordering is currently indeterministic, and may affect the test results if tests happen to be coupled. I filed a ticket a while back with Gradle, but it's not getting much traction: https://github.com/gradle/gradle/issues/8520.

I see, hopefully the Gradle team will get to it soon.

Relating to #4 and `includeAndroidResources=false`, has this been addressed yet? If not, when do we think it will be?

Yes, when includeAndroidResources=false, unit tests are already cacheable since 3.5.0-alpha03 (that's what I meant when writing comment #4).

Oh, what I meant was, when will unit tests be cacheable when it's true? You said
> Next, we are working on making unit tests cacheable when includeAndroidResources=true.

:)

Ah I see, when includeAndroidResources=true, unit tests will be cacheable starting with 3.6.0-alpha02 (comment #24).

Got it. Thanks for the clarification!