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

#2522: mockito-inline & Groovy NullPointerException #2566

Closed

Conversation

stevegreenn
Copy link

Hi!

It was requested by @TimvdLippe that I create a pull request with a new subproject that tests Groovy and mockito-inline. See here: #2522 (comment)

Unfortunately this test is currently failing as part of the ./gradlew clean build, still throwing a NullPointerException. Hopefully the new subproject can be used to track the issue down though.

Steve

@raphw
Copy link
Member

raphw commented Feb 16, 2022

Thanks, I'll see what we can do about this!

@raphw
Copy link
Member

raphw commented Mar 16, 2022

I do not get an error when running this test, it passes.

You can extract the class files using -Dnet.bytebuddy.dump=/some/folder. Could you compare the instrumented files and suggest what methods should not be instrumented?

@TimvdLippe
Copy link
Contributor

Per #2522 (comment) I think we need to update this PR to include 81bc5bd, would we not? That said, we do need the regression test to fail without any modifications and then it would need to pass with the fix. Otherwise, it is not catching the correct error.

Lastly, @stevegreenn can you run ./gradlew spotlessApply on your PR so that it gets formatted correctly?

@raphw
Copy link
Member

raphw commented Mar 17, 2022

@stevegreenn Could you rebase your PR on my change and see if the test is still failing?

@TimvdLippe
Copy link
Contributor

@nick-ernie confirmed in #2522 (comment) that 61279b2 should fix the issue.

@stevegreenn can you incorporate the above commit or would you prefer us to take in your PR and get it ready-for-merge?

@stevegreenn
Copy link
Author

Thanks for the further work on this guys! I'll get this PR updated later today.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Nice fix, thanks for bearing with us!

@TimvdLippe
Copy link
Contributor

@stevegreenn Seems like there was a compilation failure on the commit. Can you check it out and verify that ./gradlew build runs locally?

@stevegreenn
Copy link
Author

Looks like there is indeed a compilation error. From a quick glance, it looks like this is the problem:

https://github.com/mockito/mockito/pull/2566/files#diff-749d9baac17dc6e18e7db9ee686a840d65ac746dc291b591b37834ba4c8261d1R394

The method definition (found here) requires a boolean parameter, which isn't being provided in the above snippet.

@raphw - can you take a look please? I'm on holiday at the minute but if it's as simple as adding true/false there I can make that change, or I can bring in another commit again.

@TimvdLippe
Copy link
Contributor

It's probably easier if we (as maintainers) push to your branch to fix it up to get it submitted. That way, we can ensure credit on the commits and get this merged.

@raphw
Copy link
Member

raphw commented Mar 30, 2022

Was this due to a merge conflict? My branch is building fine. Maybe check out a fresh copy of my branch and cherry pick your commit on top of it?

@HonorKnight
Copy link

@TimvdLippe @stevegreenn is anyone working on this? it seems to have stagnated a bit, and this fix would be invaluable to me if it can move forward.

@raphw
Copy link
Member

raphw commented Apr 13, 2022

I cherry-picked the test project onto my branch here: #2618

Once @TimvdLippe reviews the PR, I can merge it and create a new release.

@TimvdLippe
Copy link
Contributor

This landed in #2618

@TimvdLippe TimvdLippe closed this Apr 19, 2022
@stevegreenn stevegreenn deleted the groovy-inline-test-subproject branch April 21, 2022 08:42
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.

None yet

4 participants