WitheBox.setInternalState on private static final field #673

Closed
andreicristianpetcu opened this Issue Jun 1, 2016 · 16 comments

Projects

None yet

4 participants

@andreicristianpetcu
Contributor
andreicristianpetcu commented Jun 1, 2016 edited

Hi,

I see that WitheBox.setInternalState cannot set private static final fields. Why is that? Lack of time or some other reason?

I sent a pull request to Mockito to implement this and they do not want it in Mockito. Would you accept a similar implementation in Powermock? Am I using Powermock wrong?

Thank you!

@johanhaleby
Collaborator

How did you manage to do that?

Afaik the JVM inlines this field and simply replace the value of the field at those places where it's referenced. So even if you change the field value you it won't impact the code that are using since field (at compile time at least)

@andreicristianpetcu
Contributor

I do it at runtime I don't know what JVM inlining is done behind the scenes. Here is where all the magic happens. I basically removed the final modifier from the field. My implementation leaves the fields like that but I can implement something that restores the fields modifier after the value has been set.

@thekingnothing
Member

@andreicristianpetcu,

I've added test case where you solution is failed. And for the case with String (or any primitives) you will never be able change internal state due to the fact how compiler works in this case.

I think that PowerMock already could set internal state for static final, but I'm not sure. I think so because PowerMock exactly can set static and final fields. But if not, we could accept your fix, but you need add checking field type, before make any modification in metadata. And throw exception with explanation that a user tries to make illegal action which not make sense.

@andreicristianpetcu
Contributor

That is interesting. I will investigate. Thank you!

@andreicristianpetcu andreicristianpetcu added a commit to andreicristianpetcu/powermock that referenced this issue Jun 3, 2016
@andreicristianpetcu andreicristianpetcu Fix #673: Restore fields modifiers after setInternalState a159bc0
@andreicristianpetcu andreicristianpetcu added a commit to andreicristianpetcu/powermock that referenced this issue Jun 3, 2016
@andreicristianpetcu andreicristianpetcu Fix #673: Cannot set value to static final String field 83bc044
@andreicristianpetcu andreicristianpetcu added a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
@andreicristianpetcu andreicristianpetcu Fix #673: setInternalState for public static final 3eae399
@andreicristianpetcu andreicristianpetcu added a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
@andreicristianpetcu andreicristianpetcu Fix #673: Restore fields modifiers after setInternalState cf192bf
@andreicristianpetcu andreicristianpetcu added a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
@andreicristianpetcu andreicristianpetcu Fix #673: Cannot set value to private static final primitive field 064a38b
@andreicristianpetcu andreicristianpetcu added a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
@andreicristianpetcu andreicristianpetcu Fix #673: Cannot set value to static final String field 6a2ce1d
@andreicristianpetcu
Contributor
andreicristianpetcu commented Jun 6, 2016 edited

@johanhaleby @thekingnothing Did you have the time to take a look at my pull request?

@thekingnothing
Member

@andreicristianpetcu Thank you for your pull request. I hope I'll have a time to review your pull request tomorrow.

@andreicristianpetcu
Contributor

Thank you, @thekingnothing it's good to know when you can look at it :)

@andreicristianpetcu
Contributor

@thekingnothing any update on this pull request?

@HurmuzacheCiprian

This would be very useful 👍

@andreicristianpetcu
Contributor

@thekingnothing any idea when you can look at my PR?

@thekingnothing
Member

@andreicristianpetcu I'm really sorry for so long delay. I was focus on moving my family to another country.
I've added one minor code review remark and after it'll be addressed I'll merge you change.
And again I'm really sorry for such delay.

@thekingnothing thekingnothing added this to the PowerMock 1.6.6 milestone Jun 27, 2016
@andreicristianpetcu
Contributor

@thekingnothing no problem for the delay :)

I added the changelog.txt and apparently the Travis build now fails. Apparently there are some tests failing that are unrelated to my pull request.

LargeMethodTest.largeMethodShouldBeAbleToBeSuppressed
LargeMethodTest.largeMethodShouldBeOverridden
LargeMethodTest.largeMethodShouldBeAbleToBeMocked
LargeMethodTest.largeMethodShouldBeAbleToBeMockedAndThrowException

Any clue what might have caused this? The commit that triggered the build is really small and it is in a text file.

@thekingnothing thekingnothing added a commit that closed this issue Jun 27, 2016
@andreicristianpetcu @thekingnothing andreicristianpetcu + thekingnothing Fix #673: Add support for setting private static final fields
* setInternalState for public static final
* Restore fields modifiers after setInternalState
* Cannot set value to private static final primitive field
* Cannot set value to static final String field
* updated changelog.txt setting private static final
bc2b791
@thekingnothing
Member

@andreicristianpetcu I don't have any clue what was it...but locally everything is fine and the build is okay after repeat.

@andreicristianpetcu
Contributor

@thekingnothing thank you for merging this pull request. When will the next release be published? v1.6.6

@thekingnothing
Member

@andreicristianpetcu as soon as we resolve all issue which was scheduled for next release.
https://github.com/jayway/powermock/milestones/PowerMock%201.6.6

@andreicristianpetcu
Contributor
andreicristianpetcu commented Jun 28, 2016 edited

@thekingnothing are there any of them good for beginners? Can you put a label on the ones that might be good for new contributors?

@andreicristianpetcu andreicristianpetcu added a commit to andreicristianpetcu/powermock that referenced this issue Oct 3, 2016
@andreicristianpetcu andreicristianpetcu Fix #673: Cannot set value to static final String field 60984ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment