TRUNK-5817: Migrate web and webapp package tests to Junit 5 #3392

achilep · 2020-07-22T11:02:19Z

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-5817

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

teleivo · 2020-07-22T12:01:15Z

@achilep you are trying to mock static methods with mockito. mockito does not allow you to do that. that's why we used powermock. now that we migrate to junit 5 and we cannot use powermock its not emough to just remove powermock. you need to either use reflection to set some fields that will then execute the code in a way you want in the specific test or make it an integration test. I suggest to try reflection first. for example using the spring reflectionutils you already used in the last test you migrated. if you need to get specific information on the code you are testing I suggest reaching out to @WolfSchlegel

achilep · 2020-07-22T12:16:17Z

thanks @teleivo

achilep · 2020-07-22T17:59:52Z

web/src/test/java/org/openmrs/web/filter/update/UpdateFilterModelTest.java

 		DatabaseUpdater.getUnrunDatabaseChanges(liquibaseProvider);
-		PowerMockito.verifyStatic(DatabaseUpdater.class);
+		verify(DatabaseUpdater.class);


@teleivo i try to set a changeLogDetective field ReflectionTestUtils.setField(DatabaseUpdater.class, "changeLogDetective", changeLogDetective); but i am getting this error :
java.lang.IllegalStateException: Could not access method or field: Can not set static final org.openmrs.liquibase.ChangeLogDetective field org.openmrs.util.DatabaseUpdater.changeLogDetective to org.openmrs.liquibase.ChangeLogDetective

cc @ibacher @mozzy11

its always a good idea to read the javadocs of the methods you are using. if you are talking about the spring reflectionutils then yes the javadoc says it does not support final fields. for these you have to use powermock.reflection you can find a usage in the api package. I think something like Whitebox.set...

You will then also need to adjust the dependencies of the web package. Also see the api pom.XML where I added a test score dependency for powermock.reflection and excluded the other 2 powermock dependencies.

thanks @teleivo . i will eventually used whitebox.set. I avoided using it, since we want to remove powermock dependencies.

did you have a look at the api pom? https://github.com/openmrs/openmrs-core/blob/master/api/pom.xml#L24-L45

I did exclude powermock mockito library and its junit 4 runner in the api package. what we are using is simply the powermock reflection library. since that one allows setting of static final fields.

teleivo · 2020-07-24T15:06:02Z

@achilep any luck replacing the when().then with setting fields using reflection?

achilep · 2020-07-24T20:02:42Z

thanks @teleivo , no luck. this is the last error that i try to solve : java.lang.IllegalStateException: identifying the snapshot version that had been used to initialize the OpenMRS database failed as no candidate change set resulted in zero un-run changes

teleivo · 2020-07-25T04:48:33Z

please push your latest changes and let me know which test is failing

achilep · 2020-07-25T05:13:37Z

web/src/test/java/org/openmrs/web/filter/update/UpdateFilterModelTest.java

-@PrepareForTest(DatabaseUpdater.class)
-@PowerMockIgnore({ "com.sun.org.apache.xerces.*", "javax.xml.*", "org.xml.*", "javax.management.*", "org.w3c.dom.*" })
-public class UpdateFilterModelTest {
+public class UpdateFilterModelTest  extends BaseContextSensitiveTest {


i had some error with the database connection that is why i extends BaseContextSensitiveTest

achilep · 2020-07-25T05:18:04Z

web/src/test/java/org/openmrs/web/filter/update/UpdateFilterModelTest.java

 		assertThat(model.changes, is(empty()));

-		PowerMockito.verifyStatic(DatabaseUpdater.class);
 		DatabaseUpdater.getUnrunDatabaseChanges(liquibaseProvider);


the error is with this method . i have some difficulty to manipulate the behavior of this method .

java.lang.IllegalStateException: identifying the snapshot version that had been used to initialize the OpenMRS database failed as no candidate change set resulted in zero un-run changes

the exception is cause by this line

openmrs-core/api/src/main/java/org/openmrs/liquibase/ChangeLogDetective.java

Line 116 in 02ba3ae

throw new IllegalStateException(

i have try to solve the error with this FieldUtils.writeField(changeLogDetective, "changeLogVersionFinder", changeLogVersionFinder, true); but it did not work

@WolfSchlegel can you help us out here. So with JUnit 5 PowerMock and thus mocking static methods does not work anymore. We are migrating all test to JUnit 5. This might be the last one we have to migrate. What we can do is use reflection to set static fields. Can you help us out with replacing powermock in this test with reflection so we keep its original intent.

Looking into this and apologies for the delay...

org/openmrs/web/filter/update/UpdateFilterModelTest.java is a unit test and extending BaseContextSensitiveTest makes it an integration test. I would advise against that and would revert that change first.

In the context of https://issues.openmrs.org/browse/TRUNK-4830 I wrote a few new integration tests and found that tests based on BaseContextSensitiveTestare not well isolated and understanding the behaviour of individual tests was quite hard. Eventually I created a new base class for integration tests.

@achilep could you please change the test so that it is a unit test again and ping me when this is done?

Thank you,
Wolf

Thanks, I have a look

Hey there,

please have a look at WolfSchlegel@6a08797.

I introduced a wrapper for the (mostly static) org/openmrs/util/DatabaseUpdater.java class so that PowerMock is no longer needed for mocking it. This is good news for org/openmrs/web/filter/update/UpdateFilterModelTest.java.

However, this comes at a cost as the next question is how to unit test the new wrapper without resorting to PowerMock. I gave JMockit a short try but bumped into issues that are (allegedly) resolved by rearranging the order of dependencies in the pom file (yuck) and it seems a java agent needs to be registered as well for JMockit (yuck-ish).

So I am back at square one looking for a unit test that proves that the all new org/openmrs/util/DatabaseUpdaterWrapper.java class plays nicely with the incumbent org/openmrs/util/DatabaseUpdater.java class.

To be frank, I am not sure how Java reflections or respective Spring utilities help here. At the end of the day I need to verify that the wrapper calls the (static) methods. For this purpose I introduced some static helper variables and a helper method in org/openmrs/util/DatabaseUpdater.java.

Please have a look and let me know what you think. I believe we are facing a trade-off between not testing something and adding test helpers in production code.

thank you @WolfSchlegel !

I think your suggestion for adding a wrapper is good. I added a suggestion in the code at the commit you sent.

Our goal with the UpdateModelFilterTest is to test that the filter makes sure updates are made. Your wrapper enables us to do just that. The update logic should be tested else were.

I am ok with not testing the wrapper as its super thin. This tradeoff is also ok for me since it enables us to remove powermock which hopefully forces us to rethink our designs going forward. The proliferation of static methods with side effects in the code base are a sign of bad design for me that circumvent springs dependency injection and harm testability.

Can you please bring in your changes while keeping the UpdateModelFilterTest a JUnit 4 test. We can chat about my suggestion on the code. @achilep will then do the migration to JUnit 5 once the changes are merged.

thanks @WolfSchlegel

Thanks for your feedback, @teleivo .

I removed the hand-rolled verification from the DatabaseUpdater class and moved the new wrapper to the same package that contains the UpdateFilterModel class. Making the wrapper package-private resulted in flaky Mockito behaviour when running mvn clean install so the wrapper continues to be a public class.

I created this pull request which is based on the current OpenMRS master. If you do not want to get an implicit rebase against master in mid flight, best cherry pick the relevant changes from here.

Please let me know if there is anything else I can do.

achilep · 2020-07-31T13:02:40Z

hello @WolfSchlegel , i have done . it is now a unit test

teleivo · 2020-08-02T07:00:08Z

@WolfSchlegel just to reiterate what we try to achieve: migrate this test to JUnit 5. Constraint: PowerMock is not supported with JUnit 5.

So the tools we can most likely use to replace the use of powermock are reflection either from java itself or the libraries: spring test ReflectionUtils, or powermock.reflect Whitebox (which supports setting static final fields)

If you would keep the test as JUnit 4 but remove the use of PowerMock on the current master that would already solve our issue. We can then simply turn it into JUnit 5 in a few minutes.

…ng without PowerMock

achilep · 2020-08-04T19:06:08Z

@teleivo you can merge it before we can continue .

sherrif10 · 2020-08-04T19:17:45Z

Thanks @teleivo ,@achilep can you first move it to pull request not draft if you dont mind

coveralls · 2020-08-04T20:41:47Z

Coverage decreased (-0.01%) to 63.31% when pulling 9fbdffc on achilep:TRUNK-5817-2 into f306940 on openmrs:master.

teleivo · 2020-08-05T04:50:24Z

thank you very much @WolfSchlegel !!

achilep · 2020-08-05T06:42:17Z

thank you very much @WolfSchlegel

achilep force-pushed the TRUNK-5817-2 branch from db38580 to 89b178d Compare July 22, 2020 17:52

achilep commented Jul 22, 2020

View reviewed changes

achilep force-pushed the TRUNK-5817-2 branch 2 times, most recently from a8bb03b to 70e4055 Compare July 25, 2020 05:08

achilep commented Jul 25, 2020

View reviewed changes

achilep force-pushed the TRUNK-5817-2 branch from 70e4055 to 607e1aa Compare July 31, 2020 02:07

TRUNK-5817 add wrapper for the DatabaseUpdater class to support mocki…

9fbdffc

…ng without PowerMock

achilep force-pushed the TRUNK-5817-2 branch from f21c16b to 9fbdffc Compare August 4, 2020 18:23

teleivo approved these changes Aug 5, 2020

View reviewed changes

teleivo marked this pull request as ready for review August 5, 2020 04:49

teleivo merged commit defefc8 into openmrs:master Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRUNK-5817: Migrate web and webapp package tests to Junit 5 #3392

TRUNK-5817: Migrate web and webapp package tests to Junit 5 #3392

achilep commented Jul 22, 2020

teleivo commented Jul 22, 2020

achilep commented Jul 22, 2020

achilep Jul 22, 2020

achilep Jul 22, 2020

teleivo Jul 23, 2020

achilep Jul 23, 2020 •

edited

teleivo Jul 23, 2020

teleivo commented Jul 24, 2020

achilep commented Jul 24, 2020 •

edited

teleivo commented Jul 25, 2020

achilep Jul 25, 2020

achilep Jul 25, 2020

achilep Jul 25, 2020

achilep Jul 25, 2020

teleivo Jul 25, 2020

WolfSchlegel Jul 29, 2020

WolfSchlegel Jul 31, 2020

WolfSchlegel Aug 3, 2020

teleivo Aug 4, 2020

achilep Aug 4, 2020

WolfSchlegel Aug 4, 2020 •

edited

achilep commented Jul 31, 2020

teleivo commented Aug 2, 2020

achilep commented Aug 4, 2020

sherrif10 commented Aug 4, 2020

coveralls commented Aug 4, 2020

teleivo commented Aug 5, 2020

achilep commented Aug 5, 2020

TRUNK-5817: Migrate web and webapp package tests to Junit 5 #3392

TRUNK-5817: Migrate web and webapp package tests to Junit 5 #3392

Conversation

achilep commented Jul 22, 2020

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

teleivo commented Jul 22, 2020

achilep commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achilep Jul 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teleivo commented Jul 24, 2020

achilep commented Jul 24, 2020 • edited

teleivo commented Jul 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WolfSchlegel Aug 4, 2020 • edited

Choose a reason for hiding this comment

achilep commented Jul 31, 2020

teleivo commented Aug 2, 2020

achilep commented Aug 4, 2020

sherrif10 commented Aug 4, 2020

coveralls commented Aug 4, 2020

teleivo commented Aug 5, 2020

achilep commented Aug 5, 2020

achilep Jul 23, 2020 •

edited

achilep commented Jul 24, 2020 •

edited

WolfSchlegel Aug 4, 2020 •

edited