Skip to content

Conversation

dgruntz
Copy link
Contributor

@dgruntz dgruntz commented Nov 17, 2019

As described in issue #1099 it is recommended to declare the template method as final. This pull request changes the implementation in the sample code of the following patterns:

  • template method pattern
  • twin pattern

The template pattern used in the callback pattern was already declared final (method executeWith in class com.iluwatar.callback.Task).

@dgruntz
Copy link
Contributor Author

dgruntz commented Nov 17, 2019

The reason for the failing tests is Mockito (which has problems with final methods in the version currently used, i.e. in version 1.10.19).

Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

The build is failing, and the PR can't be accepted due to that.

@iluwatar
Copy link
Owner

Is there already an open issue in Mockito about this?

@dgruntz
Copy link
Contributor Author

dgruntz commented Nov 18, 2019

The What's new in Mockito 2 document claims that it should be possible to mock final methods, but you are still using Mockito version 1.10.19. Since I do not know Mockito, I did not want to touch on that one, and I also did not want to do an easy patch by simply removing the test.

@dgruntz
Copy link
Contributor Author

dgruntz commented Nov 18, 2019

I just changed the version of Mockito, and depending on the version I get different errors in other packages, but I have to admit that I never worked with Mockito.

@dgruntz
Copy link
Contributor Author

dgruntz commented Nov 18, 2019

I changed the version of Mockito to 3.1.0 and explicitly added a dependency to bytebuddy (as read in issue1606@mockito).

This version can handle final methods. However, the test in class HalflingThiefTest still do not pass:

@Test
public void testSteal() {
  final var method = mock(StealingMethod.class);
  final var thief = new HalflingThief(method);

  thief.steal();
  verify(method).steal();

  verifyNoMoreInteractions(method); // (*)
}

The line marked with (*) will raise the following error:

[ERROR] Failures:
[ERROR]   HalflingThiefTest.testSteal:50
No interactions wanted here:
-> at com.iluwatar.templatemethod.HalflingThiefTest.testSteal(HalflingThiefTest.java:50)
But found this interaction on mock 'stealingMethod':
-> at com.iluwatar.templatemethod.StealingMethod.steal(StealingMethod.java:48)
***
For your reference, here is the list of all invocations ([?] - means unverified).
1. -> at com.iluwatar.templatemethod.StealingMethod.steal(StealingMethod.java:46)
2. [?]-> at com.iluwatar.templatemethod.StealingMethod.steal(StealingMethod.java:48)
3. [?]-> at com.iluwatar.templatemethod.StealingMethod.steal(StealingMethod.java:49)
4. [?]-> at com.iluwatar.templatemethod.StealingMethod.steal(StealingMethod.java:48)
5. [?]-> at com.iluwatar.templatemethod.StealingMethod.steal(StealingMethod.java:49)

but it is not clear to me why these additional invocations are reported.

If method StealingMethod.steal is not declared final, then the tests pass, so it has to do with the final modifier.

@dgruntz
Copy link
Contributor Author

dgruntz commented Nov 18, 2019

I opened an issue at Mockito: mockito/mockito#1826

@13998206131
Copy link

I can't understand

@dgruntz
Copy link
Contributor Author

dgruntz commented Dec 30, 2019

@13998206131 what can you not understand?

@dgruntz dgruntz mentioned this pull request Oct 14, 2020
@ohbus ohbus added the status: stale issues and pull requests that have not had recent interaction label Dec 4, 2020
@ohbus
Copy link
Contributor

ohbus commented Dec 7, 2020

@dgruntz Please comment to let us know you are still working on this.

We have assigned the issue to @elouie-code. Please have a look into this PR to get a better insight.

@iluwatar we can try building this PR again by updating the fork. What would you say?

@iluwatar
Copy link
Owner

iluwatar commented Dec 7, 2020

I opened an issue at Mockito: mockito/mockito#1826

The issue seems to be still open so I doubt the code in this PR will work. Also there's some conflicts preventing merge.

@dgruntz
Copy link
Contributor Author

dgruntz commented Dec 7, 2020

@ohbus, as @iluwatar mentioned, the issue/bug has not yet fixed (at least the issue mockito/mockito#1826 is still open).

Regarding the merge conflict: this is only a conflict with the version of mockito in the pom.xml. I upgraded mockito with this PR from version 1.10.19 to 3.1.0, but meanwhile someone else upgraded to version 3.5.6. But I will check whether the above mentioned bug has been fixed in the newest version (probably the issue has not been closed) and if this is the case, then I will adjust this PR. But as long as the bug is still part of mockito, this issue can't be fixed/merged, at leas as long as mockito is used.

@ohbus ohbus removed the status: stale issues and pull requests that have not had recent interaction label Jan 29, 2021
@dgruntz dgruntz closed this by deleting the head repository Oct 5, 2022
@dgruntz
Copy link
Contributor Author

dgruntz commented Oct 5, 2022

Replaced by PR #2057.

@iluwatar iluwatar added this to the 1.26.0 milestone Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants