Modify

Opened 3 years ago

Closed 20 months ago

Last modified 18 months ago

#18200 closed defect (fixed)

[PATCH RFC] Mocking of private methods prohibited in JMockit 1.47

Reported by: floscher Owned by: ris
Priority: normal Milestone: 20.11
Component: Unit tests Version:
Keywords: jmockit Cc:

Description

Starting with JMockit 1.47 the mocking of private methods is no longer allowed.

The methods in WindowlessMapViewStateMocker have to be modified when you want to update JMockit to a newer version:

https://josm.openstreetmap.de/browser/josm/trunk/test/unit/org/openstreetmap/josm/testutils/mockers/WindowlessMapViewStateMocker.java?rev=14052

Attachments (2)

18200.patch (7.7 KB) - added by taylor.smock 2 years ago.
Modify methods to be package instead of private, indicate via javadoc that they are visible for JMockit, update JMockit to 1.49
18200.HelpBrowserTest.patch (6.3 KB) - added by taylor.smock 20 months ago.
Fix for HelpBrowserTest

Download all attachments as: .zip

Change History (25)

comment:1 Changed 3 years ago by Don-vip

Owner: changed from team to ris

comment:2 Changed 3 years ago by Don-vip

Component: CoreUnit tests
Keywords: jmockit added

comment:3 Changed 3 years ago by ris

Christ.

comment:4 Changed 3 years ago by ris

Well https://github.com/jmockit/jmockit1/commit/b864642695bc48da5c210984aa143cb0f80eb4e0 is the commit in question and if we're desperate for a new version before this is resolved we can always maintain a very minor fork with that patch dropped. Unfortunately the reason for the removal appears to be a dogmatic belief about testing methodologies, no real technical reason.

comment:5 Changed 3 years ago by Don-vip

There was no good reasons neither to delete our comments when we reported him bug reports... We're not in a hurry, we can take the necessary time to avoid having to maintain a fork. Probably making methods protected would be enough?

comment:6 Changed 3 years ago by Don-vip

Haha I see the guy is still a moron with everyone disagreeing with him: https://github.com/jmockit/jmockit1/issues/629 I guess you won't have much success with the issue you just opened :D

comment:7 Changed 3 years ago by Don-vip

Someone created a very nice ticket explaining why this is needed in real life industrial projects: https://github.com/jmockit/jmockit1/issues/605

Changed 2 years ago by taylor.smock

Attachment: 18200.patch added

Modify methods to be package instead of private, indicate via javadoc that they are visible for JMockit, update JMockit to 1.49

comment:8 Changed 2 years ago by taylor.smock

Summary: Mocking of private methods prohibited in JMockit 1.47[PATCH RFC] Mocking of private methods prohibited in JMockit 1.47

The patch I just attached just modifies methods to be usable for JMockit 1.47+. This is for JUnit5 -- JMockit 1.46 and older don't properly clean up their mocks on JUnit >= 5.5 (JMockit 1.46 works on JUnit 5.4).

comment:9 Changed 20 months ago by simon04

Resolution: fixed
Status: newclosed

In 17090/josm:

fix #18200 - Update to JMockit 1.49 (patch by taylor.smock, modified)

comment:10 Changed 20 months ago by simon04

Milestone: 20.10

comment:11 Changed 20 months ago by simon04

Resolution: fixed
Status: closedreopened

This broke 8 unit tests: https://josm.openstreetmap.de/jenkins/job/JOSM/lastCompletedBuild/jdk=JDK8/testReport/

java.lang.IllegalArgumentException: Invalid Class argument for partial mocking (use a MockUp instead): class org.openstreetmap.josm.tools.PlatformManager

comment:12 Changed 20 months ago by ris

Only breaking 8 tests with this update is great news IMHO.

comment:13 Changed 20 months ago by simon04

Haha, your comment made my day! ;-))

Changed 20 months ago by taylor.smock

Attachment: 18200.HelpBrowserTest.patch added

Fix for HelpBrowserTest

comment:14 in reply to:  12 ; Changed 20 months ago by taylor.smock

Replying to ris:

Only breaking 8 tests with this update is great news IMHO.

In other news, I broke tests again. :(

On a more serious note, it looks very simple to fix. I attached the current proof-of-concept fix (4/8 failures) so that other people can test and make certain it works for them.

Other things to note: I switched HelpBrowserTest to only use JUnit5 methods. I'll be doing the same for the other tests I modify.

To test the attached patch (18200.HelpBrowserTest.patch), apply the patch and then run the following:
ant test-clean test '-Ddefault-junit-includes=**/HelpBrowserTest.class'

comment:15 Changed 20 months ago by simon04

In 17120/josm:

see #18200 - Fix ImageDataTest after JMockit update

comment:16 Changed 20 months ago by simon04

In 17121/josm:

see #18200 - Fix HelpBrowserTest after JMockit update (patch by taylor.smock)

comment:17 Changed 20 months ago by simon04

In 17125/josm:

see #18200 - Fix WebMarkerTest after JMockit update

comment:18 Changed 20 months ago by simon04

In 17126/josm:

see #18200 - Fix PlatformHookWindowsTest after JMockit update

comment:19 in reply to:  18 Changed 20 months ago by taylor.smock

Replying to simon04:

In 17126/josm:

see #18200 - Fix PlatformHookWindowsTest after JMockit update

You are faster than I am. :)

r17120, r17121, r17125, and r17126 should cover all of the broken tests.

comment:20 Changed 20 months ago by simon04

In 17127/josm:

see #18200 - JMockit: remove obsolete do-not-update-warning

comment:21 in reply to:  14 Changed 20 months ago by simon04

Replying to taylor.smock:

Replying to ris:

Only breaking 8 tests with this update is great news IMHO.

In other news, I broke tests again. :(

No worries. Here no user facing functionality was broken. And sometimes breaking tests give a sense of safety (proving that they really find bugs/regressions)… :-)

comment:22 Changed 20 months ago by simon04

Resolution: fixed
Status: reopenedclosed

comment:23 Changed 18 months ago by Don-vip

Milestone: 20.1020.11

Milestone renamed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain ris.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.