Skip to content
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

wrap Activity and Compose rules in LeakCanary checks on both sides #658

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RBusarow
Copy link
Collaborator

@RBusarow RBusarow commented Feb 2, 2022

I'm open to bike-shedding the name. 😄

fixes #657

@RBusarow RBusarow requested review from zach-klippenstein and a team as code owners February 2, 2022 23:02
*
* https://github.com/square/workflow-kotlin/issues/657
*/
public fun TestRule.wrapInLeakCanary(): RuleChain = requireNotNull(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual rule is just above the fold, so GitHub won't let me comment on the line I'm interested in...

try {
base.evaluate()
LeakAssertions.assertNoLeaks(tag)
} finally {
// Otherwise upstream test failures will be reported as leaks.
// https://github.com/square/leakcanary/issues/2297
AppWatcher.objectWatcher.clearWatchedObjects()
}

If we're executing this twice per test, do we want to skip the call to clearWatchedObjects() the first time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea. @py?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, should be fine. But maybe use clearAfter timestamp instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 try { 
   base.evaluate()
   val heapDumpUptimeMillis = SystemClock.uptimeMillis()
   LeakAssertions.assertNoLeaks(tag) 
 } finally { 
   // Otherwise upstream test failures will be reported as leaks. 
   // https://github.com/square/leakcanary/issues/2297 
   AppWatcher.objectWatcher.clearObjectsWatchedBefore(heapDumpUptimeMillis) 
 } 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change -- thanks!

@rjrjr rjrjr requested a review from pyricau February 2, 2022 23:31
Copy link

@vgonda vgonda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

But I'm just learning about all this, so you probably want another 👍

@RBusarow RBusarow marked this pull request as draft February 9, 2022 17:03
@RBusarow RBusarow force-pushed the rick/wrapInLeakCanary branch from 6a94bf3 to dd7b12a Compare April 19, 2022 20:58
@RBusarow RBusarow force-pushed the rick/wrapInLeakCanary branch from dd7b12a to 5b404c2 Compare April 19, 2022 21:10
@RBusarow RBusarow marked this pull request as ready for review April 19, 2022 21:39
@pyricau
Copy link
Member

pyricau commented Apr 19, 2022

One thing to note: all the changes here (and in internal forks of the rule) have been backported to LeakCanary which will soon be released as 2.9. You'll need to adapt the code a bit but should be able to delete this custom rule.

https://github.com/square/leakcanary/blob/main/leakcanary-android-instrumentation/src/main/java/leakcanary/DetectLeaksAfterTestSuccess.kt

@rjrjr
Copy link
Contributor

rjrjr commented Jun 10, 2022

@RBusarow Any plans to update this to LC 2.9 and get it merged?

@RBusarow
Copy link
Collaborator Author

@RBusarow Any plans to update this to LC 2.9 and get it merged?

Yeah, I'll get this updated soon. I took a stab at it a while ago and was running into leaks in the compose activity rule, but didn't have the time to dig into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make LeakCanary more strict
5 participants