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

PowerMock integration is still broken #2429

Closed
hzsweers opened this Issue Apr 23, 2016 · 26 comments

Comments

Projects
None yet
9 participants
@hzsweers

hzsweers commented Apr 23, 2016

Description

PowerMock has been broken in master for some time. This was first observed in January but still persists today. With the upcoming release, I want to raise this again because it's a dealbreaker for many consumers of the library. I think it should be considered a blocker for releasing 3.1.

CC @jongerrish @karlicoss

Steps to Reproduce

See existing issue: #2208
Existing PRs: #2390, #2213

Robolectric & Android Version

Robolectric 3.1-SNAPSHOT, all versions

@erd erd added defect 3.1 labels Apr 24, 2016

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss

karlicoss Apr 30, 2016

Contributor

Yeah, agree. Unfortunately, I am kinda stuck in my PRs with some weird things I don't understand :( Would be great if someone more familiar with powermock core could take a look at #2390

Contributor

karlicoss commented Apr 30, 2016

Yeah, agree. Unfortunately, I am kinda stuck in my PRs with some weird things I don't understand :( Would be great if someone more familiar with powermock core could take a look at #2390

@alxndrsn alxndrsn referenced this issue May 3, 2016

Closed

Support API 23 #1865

@erd

This comment has been minimized.

Show comment
Hide comment
@erd

erd May 19, 2016

Member

I would like to fix this before finalizing 3.1, but I don't know much about PowerMock. Could someone else here help out with a PR?

Member

erd commented May 19, 2016

I would like to fix this before finalizing 3.1, but I don't know much about PowerMock. Could someone else here help out with a PR?

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish May 19, 2016

Contributor

I have a theory that it is due to our caching of classloaders between tests.

I'd like to rewrite this code so that we start with a fresh classloader for each test (we can cache the class byte stream but not the class definition). I think this would also remove the need for @resetter logic so would give a cleaner environment, it would also stop leakages on the heap I think in some cases.

I honestly think we should still push 3.1 regardless as I doubt most people are using Power Mock and a release is way over due, but we can plan on a 3.1 release with this fix in and also address the performance regressions reported.

Thoughts?

Contributor

jongerrish commented May 19, 2016

I have a theory that it is due to our caching of classloaders between tests.

I'd like to rewrite this code so that we start with a fresh classloader for each test (we can cache the class byte stream but not the class definition). I think this would also remove the need for @resetter logic so would give a cleaner environment, it would also stop leakages on the heap I think in some cases.

I honestly think we should still push 3.1 regardless as I doubt most people are using Power Mock and a release is way over due, but we can plan on a 3.1 release with this fix in and also address the performance regressions reported.

Thoughts?

@hzsweers

This comment has been minimized.

Show comment
Hide comment
@hzsweers

hzsweers May 19, 2016

We are using power mock and have ~120 engineers. I think it's a regression
and should be fixed, but I'm not sure how much of a say I have since I
don't really know what's required to fix it. We wouldn't be able to update
until power mock integration is fixed :/

On Wed, May 18, 2016, 7:40 PM Jonathan Gerrish notifications@github.com
wrote:

I have a theory that it is due to our caching of classloaders between
tests.

I'd like to rewrite this code so that we start with a fresh classloader
for each test (we can cache the class byte stream but not the class
definition). I think this would also remove the need for @resetter
https://github.com/Resetter logic so would give a cleaner environment,
it would also stop leakages on the heap I think in some cases.

I honestly think we should still push 3.1 regardless as I doubt most
people are using Power Mock and a release is way over due, but we can plan
on a 3.1 release with this fix in and also address the performance
regressions reported.

Thoughts?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#2429 (comment)

hzsweers commented May 19, 2016

We are using power mock and have ~120 engineers. I think it's a regression
and should be fixed, but I'm not sure how much of a say I have since I
don't really know what's required to fix it. We wouldn't be able to update
until power mock integration is fixed :/

On Wed, May 18, 2016, 7:40 PM Jonathan Gerrish notifications@github.com
wrote:

I have a theory that it is due to our caching of classloaders between
tests.

I'd like to rewrite this code so that we start with a fresh classloader
for each test (we can cache the class byte stream but not the class
definition). I think this would also remove the need for @resetter
https://github.com/Resetter logic so would give a cleaner environment,
it would also stop leakages on the heap I think in some cases.

I honestly think we should still push 3.1 regardless as I doubt most
people are using Power Mock and a release is way over due, but we can plan
on a 3.1 release with this fix in and also address the performance
regressions reported.

Thoughts?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#2429 (comment)

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish May 19, 2016

Contributor

Ya, its definitely a regression.

We don't really use Power Mock right now, but I'd rather promote teams use Power Mock over writing custom shadows for their own code, so I'm certainly in the camp of fixing it asap.

The question is do we hold up an already very late 3.1 release on this issue as I'm guessing most teams not using Power Mock would want an updated non-snapshot version of RE. I want to fix it, and its next on my list as soon as I've upgraded out test corpus to work with the latest snapshot, there are a lot of breakages due to the shadow code removed from Context/ShadowContext etc.

I'd like to get a major RE release to coincide with every Android release, then have regular minor releases with a clear deprecation policy and API change documentation.

Contributor

jongerrish commented May 19, 2016

Ya, its definitely a regression.

We don't really use Power Mock right now, but I'd rather promote teams use Power Mock over writing custom shadows for their own code, so I'm certainly in the camp of fixing it asap.

The question is do we hold up an already very late 3.1 release on this issue as I'm guessing most teams not using Power Mock would want an updated non-snapshot version of RE. I want to fix it, and its next on my list as soon as I've upgraded out test corpus to work with the latest snapshot, there are a lot of breakages due to the shadow code removed from Context/ShadowContext etc.

I'd like to get a major RE release to coincide with every Android release, then have regular minor releases with a clear deprecation policy and API change documentation.

@hzsweers

This comment has been minimized.

Show comment
Hide comment
@hzsweers

hzsweers May 25, 2016

Actually there's still a Wiki page dedicated to use of powermock. I'd say that enough to call it officially supported: https://github.com/robolectric/robolectric/wiki/Using-PowerMock.

hzsweers commented May 25, 2016

Actually there's still a Wiki page dedicated to use of powermock. I'd say that enough to call it officially supported: https://github.com/robolectric/robolectric/wiki/Using-PowerMock.

@kingargyle

This comment has been minimized.

Show comment
Hide comment
@kingargyle

kingargyle Jun 2, 2016

Contributor

I suggest that you push 3.1 and then target this for a 3.1.1 bug fix release and document it as a known issue for 3.1.

Contributor

kingargyle commented Jun 2, 2016

I suggest that you push 3.1 and then target this for a 3.1.1 bug fix release and document it as a known issue for 3.1.

@hzsweers

This comment has been minimized.

Show comment
Hide comment
@hzsweers

hzsweers Jun 7, 2016

Any plans to document it? Current release notes don't mention it

hzsweers commented Jun 7, 2016

Any plans to document it? Current release notes don't mention it

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss

karlicoss Jun 7, 2016

Contributor

Agree, should definitely be in the release notes

Contributor

karlicoss commented Jun 7, 2016

Agree, should definitely be in the release notes

@erd

This comment has been minimized.

Show comment
Hide comment
@erd

erd Jun 7, 2016

Member

I added a "Known Issues" section with a link to this issue.

Member

erd commented Jun 7, 2016

I added a "Known Issues" section with a link to this issue.

@mplacona

This comment has been minimized.

Show comment
Hide comment
@mplacona

mplacona Jul 10, 2016

Has there been any updates or workarounds on #2429? This is really coming on the way now.

Appreciate any input.

mplacona commented Jul 10, 2016

Has there been any updates or workarounds on #2429? This is really coming on the way now.

Appreciate any input.

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish Jul 10, 2016

Contributor

We'll be taking a look at this next on the list but there are a couple of
tasks to finish first.

On Jul 10, 2016 11:09 AM, "Marcos Placona" notifications@github.com wrote:

Has there been any updates or workarounds on #2429
#2429? This is really
coming on the way now.

Appreciate any input.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2429 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGBaeMxoKLfvuLBNKoJGNzNxPpF54XeFks5qUTVwgaJpZM4IOSW0
.

Contributor

jongerrish commented Jul 10, 2016

We'll be taking a look at this next on the list but there are a couple of
tasks to finish first.

On Jul 10, 2016 11:09 AM, "Marcos Placona" notifications@github.com wrote:

Has there been any updates or workarounds on #2429
#2429? This is really
coming on the way now.

Appreciate any input.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2429 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGBaeMxoKLfvuLBNKoJGNzNxPpF54XeFks5qUTVwgaJpZM4IOSW0
.

@mplacona

This comment has been minimized.

Show comment
Hide comment
@mplacona

mplacona Jul 10, 2016

Thanks @jongerrish! Any ideas if there are any suggested workarounds on the time being?

mplacona commented Jul 10, 2016

Thanks @jongerrish! Any ideas if there are any suggested workarounds on the time being?

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish Jul 10, 2016

Contributor

You can refactor your code to not use statics, then you can inject objects
and mock them with regular Mockito.

On Jul 10, 2016 11:13 AM, "Marcos Placona" notifications@github.com wrote:

Thanks @jongerrish https://github.com/jongerrish! Any ideas if there
are any suggested workarounds on the time being?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2429 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGBaePjMKjFenJQe5opoGvBT3wiNXHY3ks5qUTZBgaJpZM4IOSW0
.

Contributor

jongerrish commented Jul 10, 2016

You can refactor your code to not use statics, then you can inject objects
and mock them with regular Mockito.

On Jul 10, 2016 11:13 AM, "Marcos Placona" notifications@github.com wrote:

Thanks @jongerrish https://github.com/jongerrish! Any ideas if there
are any suggested workarounds on the time being?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2429 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGBaePjMKjFenJQe5opoGvBT3wiNXHY3ks5qUTZBgaJpZM4IOSW0
.

@hzsweers

This comment has been minimized.

Show comment
Hide comment
@hzsweers

hzsweers Jul 27, 2016

3.1.1 is out with no changelog, assuming this was not fixed in it?

hzsweers commented Jul 27, 2016

3.1.1 is out with no changelog, assuming this was not fixed in it?

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish Jul 27, 2016

Contributor

No it wasn't. There are other issues that we're working on first. I'd really welcome a contribution though to fix this, it is a priority and I'm happy to give guidance.

Contributor

jongerrish commented Jul 27, 2016

No it wasn't. There are other issues that we're working on first. I'd really welcome a contribution though to fix this, it is a priority and I'm happy to give guidance.

@hzsweers

This comment has been minimized.

Show comment
Hide comment
@hzsweers

hzsweers Jul 27, 2016

What would you recommend someone looking into this do?

hzsweers commented Jul 27, 2016

What would you recommend someone looking into this do?

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish Jul 27, 2016

Contributor

Someone has already taken a look at this and got stuck, but there is an open PR they left...

#2390

I would sync with the author of this @karlicoss

I think the issue might be that we are caching the classloader between API versions and some of the mock class definitions are getting shared between them. Could be a good idea not to cache classloaders between tests although this will result in a performance regression, but there may be other ways to make that back to some degree, e,g: caching the class bytes read from disk before the classes are loaded, rather than then class definitions themselves?

Contributor

jongerrish commented Jul 27, 2016

Someone has already taken a look at this and got stuck, but there is an open PR they left...

#2390

I would sync with the author of this @karlicoss

I think the issue might be that we are caching the classloader between API versions and some of the mock class definitions are getting shared between them. Could be a good idea not to cache classloaders between tests although this will result in a performance regression, but there may be other ways to make that back to some degree, e,g: caching the class bytes read from disk before the classes are loaded, rather than then class definitions themselves?

@karlicoss

This comment has been minimized.

Show comment
Hide comment
@karlicoss

karlicoss Jul 27, 2016

Contributor

@jongerrish yeah, I've tried to fix this few months ago, but everything I figured out about the cause, I put in the comments. So yeah, it was some issue with classloaders caching, but that's where I stuck and kinda gave up.

Contributor

karlicoss commented Jul 27, 2016

@jongerrish yeah, I've tried to fix this few months ago, but everything I figured out about the cause, I put in the comments. So yeah, it was some issue with classloaders caching, but that's where I stuck and kinda gave up.

@mohib-irshad-10p

This comment has been minimized.

Show comment
Hide comment
@mohib-irshad-10p

mohib-irshad-10p Dec 7, 2016

Is this fixed? Really need to use Powermock.. :(

mohib-irshad-10p commented Dec 7, 2016

Is this fixed? Really need to use Powermock.. :(

@abangkis

This comment has been minimized.

Show comment
Hide comment
@abangkis

abangkis Dec 20, 2016

Is there any update for this issue? I see it still not assigned to anyone?

abangkis commented Dec 20, 2016

Is there any update for this issue? I see it still not assigned to anyone?

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish Dec 20, 2016

Contributor

Hi,

Sorry for the lack of activity on this issue. The update is that this is a priority for us and we'll get to it early January. Our #1 priority right now is the next major release which will deliver api 24 + 25 support as well as the resources fix for the issue that prevented using AppCompat on older runtimes. There is still a corner case bug in the resource handling that we are working on but once this is fixed we'll cut a new major release.

This issue will be our #1 priority right after this which we will include in a point release.

Sorry for the delay, I understand its frustrating.

Contributor

jongerrish commented Dec 20, 2016

Hi,

Sorry for the lack of activity on this issue. The update is that this is a priority for us and we'll get to it early January. Our #1 priority right now is the next major release which will deliver api 24 + 25 support as well as the resources fix for the issue that prevented using AppCompat on older runtimes. There is still a corner case bug in the resource handling that we are working on but once this is fixed we'll cut a new major release.

This issue will be our #1 priority right after this which we will include in a point release.

Sorry for the delay, I understand its frustrating.

@kingargyle

This comment has been minimized.

Show comment
Hide comment
@kingargyle

kingargyle Dec 20, 2016

Contributor

@jongerrish you probably want to take a look at the classloader fix that is reference in the Mockito 2.x compatiblity bug elsewhere. It may fix the PowerMock issues here as well.

Contributor

kingargyle commented Dec 20, 2016

@jongerrish you probably want to take a look at the classloader fix that is reference in the Mockito 2.x compatiblity bug elsewhere. It may fix the PowerMock issues here as well.

@jongerrish

This comment has been minimized.

Show comment
Hide comment
@jongerrish

jongerrish Dec 20, 2016

Contributor

@kingargyle - Thanks, yeah, I'm aware of it, I also suspect its related, we're going to work on both of these issues.

Contributor

jongerrish commented Dec 20, 2016

@kingargyle - Thanks, yeah, I'm aware of it, I also suspect its related, we're going to work on both of these issues.

@xian

This comment has been minimized.

Show comment
Hide comment
@xian

xian Jan 5, 2017

Member

Dupe of: #2677 [EDIT: actually it's not]

Member

xian commented Jan 5, 2017

Dupe of: #2677 [EDIT: actually it's not]

@xian xian closed this Jan 5, 2017

@xian xian reopened this Jan 6, 2017

@xian xian added this to the 3.2.1 milestone Jan 6, 2017

@xian xian removed the 3.1 label Jan 6, 2017

@xian

This comment has been minimized.

Show comment
Hide comment
@xian

xian Jan 6, 2017

Member

Dupe of #2208.

Member

xian commented Jan 6, 2017

Dupe of #2208.

@xian xian closed this Jan 6, 2017

@xian xian removed this from the 3.2.1 milestone Jan 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment