Modify

Opened 17 months ago

Closed 5 months ago

Last modified 3 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 8 months 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 5 months ago.
Fix for HelpBrowserTest

Download all attachments as: .zip

Change History (25)

comment:1 Changed 17 months ago by Don-vip

Owner: changed from team to ris

comment:2 Changed 17 months ago by Don-vip

Component: CoreUnit tests
Keywords: jmockit added

comment:3 Changed 17 months ago by ris

Christ.

comment:4 Changed 17 months 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 17 months 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 17 months 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 17 months 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 8 months 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 8 months 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 5 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 5 months ago by simon04

Milestone: 20.10

comment:11 Changed 5 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 5 months ago by ris

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

comment:13 Changed 5 months ago by simon04

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

Changed 5 months ago by taylor.smock

Attachment: 18200.HelpBrowserTest.patch added

Fix for HelpBrowserTest

comment:14 in reply to:  12 ; Changed 5 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 5 months ago by simon04

In 17120/josm:

see #18200 - Fix ImageDataTest after JMockit update

comment:16 Changed 5 months ago by simon04

In 17121/josm:

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

comment:17 Changed 5 months ago by simon04

In 17125/josm:

see #18200 - Fix WebMarkerTest after JMockit update

comment:18 Changed 5 months ago by simon04

In 17126/josm:

see #18200 - Fix PlatformHookWindowsTest after JMockit update

comment:19 in reply to:  18 Changed 5 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 5 months ago by simon04

In 17127/josm:

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

comment:21 in reply to:  14 Changed 5 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 5 months ago by simon04

Resolution: fixed
Status: reopenedclosed

comment:23 Changed 3 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.