The Wayback Machine - https://web.archive.org/web/20201206183238/https://github.com/square/leakcanary/issues/1986
Skip to content

/ leakcanary

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

Jump to bottom

Leakcanary makes test always pass when using android-test 1.3.1-alpha02 #1986

Open
mateuszkwiecinski opened this issue Nov 2, 2020 · 7 comments
Open

Leakcanary makes test always pass when using android-test 1.3.1-alpha02 #1986

mateuszkwiecinski opened this issue Nov 2, 2020 · 7 comments
Labels
good first issue status: help wanted type: bug

Comments

@mateuszkwiecinski
Copy link

@mateuszkwiecinski mateuszkwiecinski commented Nov 2, 2020 •
edited

Description

When using leakcanary with android-test 1.3.1-alpha02 ui test suite always pass.
Using alpha version is required due to android/android-test#743 but in case when it gets into stable release more people will face the issue 😞

Steps to Reproduce

Sample project that has test suite passing despite it shouldn't
mateuszkwiecinski/orchestrator_doesnt_work#7

image

The repository contains simple ui tests setup with orchestrator and leakcananry enabled

Expected behavior: I expect to see test failure and have failing workflow (the test shouldFail should fail)

Version Information

  • LeakCanary version: 2.5
  • Android OS version: tested 26, 29, 30
  • Gradle version: tested on 6.6.1 and 6.7

Additional Information

When looking at logcat it points at leakcanary:

2020-11-02 11:05:03.546 470-540/com.package.debug E/TestRunner: ----- begin exception -----
2020-11-02 11:05:03.546 470-540/com.package.debug E/TestRunner: kotlin.UninitializedPropertyAccessException: lateinit property testResultPublisher has not been initialized
        at leakcanary.FailTestOnLeakRunListener.testFinished(FailTestOnLeakRunListener.kt:134)
        at org.junit.runner.notification.SynchronizedRunListener.testFinished(SynchronizedRunListener.java:87)
        at org.junit.runner.notification.RunNotifier$9.notifyListener(RunNotifier.java:225)
        at org.junit.runner.notification.RunNotifier$SafeNotifier.run(RunNotifier.java:72)
        at org.junit.runner.notification.RunNotifier.fireTestFinished(RunNotifier.java:222)
        at org.junit.internal.runners.model.EachTestNotifier.fireTestFinished(EachTestNotifier.java:38)
        at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:372)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
        at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
        at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
        at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
        at org.junit.runners.Suite.runChild(Suite.java:128)
        at org.junit.runners.Suite.runChild(Suite.java:27)
        at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
        at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
        at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
        at androidx.test.internal.runner.TestExecutor.execute(TestExecutor.java:56)
        at androidx.test.runner.AndroidJUnitRunner.onStart(AndroidJUnitRunner.java:434)
        at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2205)
2020-11-02 11:05:03.546 470-540/compackage.debug E/TestRunner: ----- end exception -----
@mateuszkwiecinski mateuszkwiecinski mentioned this issue Nov 2, 2020
Closed
@mateuszkwiecinski mateuszkwiecinski mentioned this issue Nov 4, 2020
Open
@opatry
Copy link
Contributor

@opatry opatry commented Nov 7, 2020

lateinit var are so Java's NPE… I tend to avoid it and simply rely on nullable far more explicit!

I suggest to make testResultPublisher nullable then.
Otherwise, check on this::testResultPublisher.isInitialized could be done but meehh…
opatry added a commit to opatry/leakcanary that referenced this issue Nov 7, 2020
@opatry
Fix potential when using (fixes square#1986)
0ba80b4
@opatry
Copy link
Contributor

@opatry opatry commented Nov 7, 2020

@pyricau unless this proposal is considered too defensive and this state isn't expected when executing RunListener implementation?

Is it something possible to have testRunStarted not called while failTest (called by detectLeaks from testFinished) is?
@opatry
Copy link
Contributor

@opatry opatry commented Nov 7, 2020

@pyricau do you have a recipe somewhere to use leakcanary from sources in an external project, like the one provided in this issue? Currently, I tweaked a lot (both on leakcanary and example repositories) to make it work.
So, I guess I'm missing something obvious 😅
@opatry
Copy link
Contributor

@opatry opatry commented Nov 7, 2020 •
edited

The real issue causing trouble is the following exception, not kotlin.UninitializedPropertyAccessException: lateinit property testResultPublisher has not been initialized when I try with my setup:

2020-11-07 22:10:32.129 14094-14119/com.example.myapplication E/TestRunner: failed: Test mechanism
2020-11-07 22:10:32.129 14094-14119/com.example.myapplication E/TestRunner: ----- begin exception -----
2020-11-07 22:10:32.130 14094-14119/com.example.myapplication E/TestRunner: java.lang.NoSuchFieldException: No field orchestratorListener in class Landroidx/test/runner/AndroidJUnitRunner; (declaration of 'androidx.test.runner.AndroidJUnitRunner' appears in /data/app/~~CPf-cpXbvJK3gl7Eclskvg==/com.example.myapplication.test-IBmvTiaApQAoie0dWx3Nyg==/base.apk)
        at java.lang.Class.getDeclaredField(Native Method)
        at leakcanary.internal.TestResultPublisher$Companion.install(TestResultPublisher.kt:22)
        at leakcanary.FailTestOnLeakRunListener.testRunStarted(FailTestOnLeakRunListener.kt:63)
        at org.junit.runner.notification.SynchronizedRunListener.testRunStarted(SynchronizedRunListener.java:35)
        at org.junit.runner.notification.RunNotifier$1.notifyListener(RunNotifier.java:91)
        at org.junit.runner.notification.RunNotifier$SafeNotifier.run(RunNotifier.java:72)
        at org.junit.runner.notification.RunNotifier.fireTestRunStarted(RunNotifier.java:88)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:136)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
        at androidx.test.internal.runner.TestExecutor.execute(TestExecutor.java:56)
        at androidx.test.runner.AndroidJUnitRunner.onStart(AndroidJUnitRunner.java:434)
        at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2205)
2020-11-07 22:10:32.130 14094-14119/com.example.myapplication E/TestRunner: ----- end exception -----
2020-11-07 22:10:32.131 14094-14119/com.example.myapplication E/OrchestrationListener: Unable to determine test case from description [No Tests]
    androidx.test.services.events.TestEventException: Unexpected description instance: No Tests
        at androidx.test.services.events.ParcelableConverter.getTestCaseFromDescription(ParcelableConverter.java:49)
        at androidx.test.internal.events.client.OrchestratedInstrumentationListener.getTestFailureEventFromCachedDescription(OrchestratedInstrumentationListener.java:156)
        at androidx.test.internal.events.client.OrchestratedInstrumentationListener.testFailure(OrchestratedInstrumentationListener.java:137)
        at org.junit.runner.notification.SynchronizedRunListener.testFailure(SynchronizedRunListener.java:63)
        at org.junit.runner.notification.RunNotifier$4.notifyListener(RunNotifier.java:142)
        at org.junit.runner.notification.RunNotifier$SafeNotifier.run(RunNotifier.java:72)
        at org.junit.runner.notification.RunNotifier.fireTestFailures(RunNotifier.java:138)
        at org.junit.runner.notification.RunNotifier.access$100(RunNotifier.java:21)
        at org.junit.runner.notification.RunNotifier$SafeNotifier.run(RunNotifier.java:78)
        at org.junit.runner.notification.RunNotifier.fireTestRunStarted(RunNotifier.java:88)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:136)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
        at androidx.test.internal.runner.TestExecutor.execute(TestExecutor.java:56)
        at androidx.test.runner.AndroidJUnitRunner.onStart(AndroidJUnitRunner.java:434)
        at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2205)

I protected such error and the test is now correctly marked as failed as expected.
That being said, I don't know exactly what it means and if something more needs to be done. I guess this access by reflection wasn't made for the pleasure of doing it. Which concrete instance of AndroidJUnitRunner are you expecting that should have a orchestratorListener field?

I'll push a PR with my changes even if I don't think it will be the end of the story.
opatry added a commit to opatry/leakcanary that referenced this issue Nov 7, 2020
@opatry
Fix potential `UninitializedPropertyAccessException` when using `Fail…
d459676 
…TestOnLeakRunListener .testResultPublisher` (relates to square#1986)

This seems to be the cause of square#1986 but even without this fix, the issue example worked by fixing the root cause of this issue.
opatry added a commit to opatry/leakcanary that referenced this issue Nov 7, 2020
@opatry
Protect against `NoSuchFieldException` when looking for `orchestrator…
e649f36 
…Listener` field (fixes square#1986)

Such field might be missing, observed with `android-test 1.3.1-alpha02` at least.
@opatry opatry mentioned this issue Nov 7, 2020
Open
@opatry
Copy link
Contributor

@opatry opatry commented Nov 7, 2020

It seems this commit android/android-test/e998fe63c1870e79548d2b6386db81ca07010c5c introduced the issue.

OrchestratedInstrumentationListener orchestratorListener is replaced by TestEventClient testEventClient.

TestEventClient is only available with androidx.test:runner:1.3.1-alpha02, if leakcanary needs to depend on it, is it a problem?
Moreover, OrchestratedInstrumentationListener package changed from androidx.test.orchestrator.instrumentationlistener to androidx.test.internal.events.client, so I guess having a strategy for old runtime and recent one might be difficult with current impl.
Might need deeper changes I'm afraid.
@pyricau
Copy link
Member

@pyricau pyricau commented Dec 2, 2020

Catching up, sorry it took me so long to take a look.
@pyricau
Copy link
Member

@pyricau pyricau commented Dec 3, 2020

do you have a recipe somewhere to use leakcanary from sources in an external project, like the one provided in this issue?

The LeakCanary example app has UI tests that fail (and don't run on CI). You could add a test there (and update the espresso version temporarily).

Based on what you're telling me, it sounds like we need a two different impls. We could check for whether the OrchestratedInstrumentationListener vs TestEventClient impls are available, and use one strategy / test impl vs another. We'd have to move those impls into two different modules that each have a different version of the dependency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue status: help wanted type: bug
3 participants
@opatry @pyricau @mateuszkwiecinski
You can’t perform that action at this time.