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

WitheBox.setInternalState on private static final field #673

Closed
andreicristianpetcu opened this issue Jun 1, 2016 · 23 comments
Closed

WitheBox.setInternalState on private static final field #673

andreicristianpetcu opened this issue Jun 1, 2016 · 23 comments

Comments

@andreicristianpetcu
Copy link
Contributor

andreicristianpetcu commented Jun 1, 2016

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
Copy link
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
Copy link
Contributor Author

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.

@thekingn0thing
Copy link
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
Copy link
Contributor Author

That is interesting. I will investigate. Thank you!

andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 3, 2016
andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 3, 2016
andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 3, 2016
andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 3, 2016
andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
andreicristianpetcu pushed a commit to andreicristianpetcu/powermock that referenced this issue Jun 4, 2016
@andreicristianpetcu
Copy link
Contributor Author

andreicristianpetcu commented Jun 6, 2016

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

@thekingn0thing
Copy link
Member

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

@andreicristianpetcu
Copy link
Contributor Author

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

@andreicristianpetcu
Copy link
Contributor Author

@thekingnothing any update on this pull request?

@HurmuzacheCiprian
Copy link

This would be very useful 👍

@andreicristianpetcu
Copy link
Contributor Author

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

@thekingn0thing
Copy link
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.

@andreicristianpetcu
Copy link
Contributor Author

@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.

@thekingn0thing
Copy link
Member

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

@andreicristianpetcu
Copy link
Contributor Author

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

@thekingn0thing
Copy link
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
Copy link
Contributor Author

andreicristianpetcu commented Jun 28, 2016

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

@rea-al
Copy link

rea-al commented Feb 7, 2019

Is this added in PowerMock 1.7.4?

@andreicristianpetcu
Copy link
Contributor Author

yes it is in 1.7.4

@rea-al
Copy link

rea-al commented Feb 8, 2019

Weird... I am getting next issue:

org.powermock.reflect.exceptions.FieldNotFoundException: No instance field assignable from "org.apache.logging.log4j.Logger" could be found in the class hierarchy of Foo.

While running next test:

public class Foo {
    private static final Logger LOG = LogManager.getLogger(Foo.class.getName());
}

public class FooTest {
    @Rule
    public MockitoRule mockitoRule = MockitoJUnit.rule();

    @Mock
    Logger logger;

    @Test
    public void test() {
        Foo sut = new Foo();
        
        Whitebox.setInternalState(sut, Logger.class, logger);
    }
}

With the following dependencies on JDK 1.8:

  • junit:junit:4.12
  • org.mockito:mockito-core:2.23.0
  • org.powermock:powermock-module-junit4:1.7.4
  • org.powermock:powermock-api-mockito2:1.7.4

Do you mind to confirm if something is wrong @andreicristianpetcu ?

@andreicristianpetcu
Copy link
Contributor Author

I don't use powermock anymore. I don't work for the company that I used to work for when I contributed this change. I try to limit my time invested in free software. If I were you, I would report a new issue, clone this project and debug it locally. I know very little about this project. I basically copy/pasted from stack overflow into this repo :D

@rainbow702
Copy link

@andreicristianpetcu

Could you please take a look at this issue.

I would be thankful for any help.

@andreicristianpetcu
Copy link
Contributor Author

@rainbow702 the compiler inlines primitives and strings. Integer is an object, not a primitive. I cannot change something inlined by the compiler.

Try avoiding this or getting the String object and modify it's internal state.

@rainbow702
Copy link

@andreicristianpetcu

thanks

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

No branches or pull requests

6 participants