-
Notifications
You must be signed in to change notification settings - Fork 476
Fix SpyStatic() with an interaction closure throws NullPointerException #2254
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
3c5beab
Fix SpyStatic() with an interaction closure throws NullPointerException
AndreasTu 791bcfc
Merge branch 'master' into SpyStatic
AndreasTu f96bb0e
Changed the implementation to now use new SpyStatic() API.
AndreasTu 6d77ab0
Fixup test for Groovy 5
AndreasTu 9751a3e
Revert the addition of the method to IMockMakerSettings because it wo…
AndreasTu e140460
Added test for non-static exception.
AndreasTu f3747e6
Added test for non-static exception.
AndreasTu 9b1176e
Merge branch 'master' into SpyStatic
leonard84 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this misleading? IMO, this adds a feature that didn't exist before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does. I am not really convinced that a user should use the new syntax:
because it does not work so great, due to the Class instance issues, e.g. IDE support.
The better syntax is the documented one, without any closure.
As I said, I can also remove the new syntax and add a second method to
IMockMakerSettingsto prevent the auto-conversion, if you like that better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leonard84 I have changed PR to not use new API, you can have a look what you like better :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it again, because the new method to
IMockMakerSettingswould break contribution API for external MockMakers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why do we not support something like
like we do for all other mock objects?
Imho it should be up to the user whether he does that or
like is supported for all other mock objects.
And it should solve the problem, as with a
Closureoverload, that should be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leonard sounded that he was not so convinced about that new API, especially the lack of IDE support.
So I made a non-API change bugfix.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just treat it like an instance is given in?
Yes, you then get also instance methods completed, but as you can also call static methods on instances, you also get the static methods.
On the other mock closures you also get both while calling the static does not really make sense.
So I'd say just add the
@ClosureParamsand@DelegatesToannotations and add an?: enclosingCall('SpyStatic')to the gdsl and whatever is necessary for the Eclipse-version and it should be fine enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't you get IDE warnings that static methods are called on an instance?
Actually before this, I would perfer the nothing at all as in my commit and the user shall use the class syntax inside the closure, which works fine and completes correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the IDE can warn about this, because due to the duck-typing of Groovy, the IDE cannot know whether the instance might also have an instance method with that name at runtime. If it complains, I'd say that is an IDE bug.
But actually I do not see a warning even with
But you can also do
and then the type is correctly considered to be
Class<StaticClass>and the static methods are completed as expected too.And the
?: enclosingCall('StaticSpy')in thespock.gdslshould also work as expected, as it should also deliverClass<StaticClass>as type and thus be correct.For the
spock_tests.dsldI'd expect the same.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my comment was more about the
Fixpart. The way it is written, you fixed something that didn't exist before, so the more appropriate line would beAdd <feature>.Now with the feature removed, the
Fixprefix makes sense again.To be clear, I'm not fundamentally opposed to an initializing closure.
I'm going to merge this as-is to hopefully release M7 today.