UIFR-222: Adding Java 17 Compatability #76

wikumChamith · 2023-12-04T17:48:20Z

Ticket: https://openmrs.atlassian.net/browse/UIFR-222

dkayiwa · 2023-12-04T18:25:02Z

api/src/test/java/org/openmrs/ui/framework/converter/StringToGlobalPropertyConverterTest.java

@@ -11,7 +11,7 @@ public class StringToGlobalPropertyConverterTest extends BaseModuleContextSensit
 	public void convert_shouldConvertStringToGlobalProperty() {
 		StringToGlobalPropertyConverter converter = new StringToGlobalPropertyConverter();
 		GlobalProperty prop = converter.convert("locale.allowed.list");
-		Assert.assertEquals("en", prop.getPropertyValue());
+		Assert.assertEquals("en_GB", prop.getPropertyValue());


Did you change this accidentally?

After updating to platform 2.0.0 this test failed with the following error:

junit.framework.ComparisonFailure: expected:<en[]> but was:<en[_GB]> at org.openmrs.ui.framework.converter.StringToGlobalPropertyConverterTest.convert_shouldConvertStringToGlobalProperty

That's why I updated the test.

OK. That makes sense. 😊

dkayiwa · 2023-12-04T18:25:50Z

pom.xml

+
+        <dependency>
+            <groupId>com.thoughtworks.xstream</groupId>
+            <artifactId>xstream</artifactId>


Did you intentionally leave out scope?

I added the scope

dkayiwa · 2023-12-04T18:37:11Z

pom.xml

@@ -262,7 +262,7 @@
                    <artifactId>maven-surefire-plugin</artifactId>
                    <version>2.22.1</version>
                    <configuration>
-                        <argLine>-Xmx512m -XX:MaxPermSize=512m -Djdk.net.URLClassPath.disableClassPathURLCheck=true</argLine>


Was this change also needed by the tests to pass?

I think we can keep this. Should I revert this?

dkayiwa · 2023-12-04T18:44:18Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,39 @@
+<!--- Add a pull request title above in this format -->
+<!--- real example: 'RESTWS-738 Remove concept property setters from ObsResource classes' -->
+<!--- 'RESTWS-JiraIssueNumber JiraIssueTitle' -->


Was this also part of the changes needed to make it compile on Java 17?

I tried to kill two birds with one stone :)

Did you get a chance to look at this? https://opensource.com/article/18/6/anatomy-perfect-pull-request

dkayiwa · 2023-12-04T18:57:06Z

api/src/test/java/org/openmrs/ui/framework/formatter/FormatterServiceTest.java

@@ -63,11 +62,10 @@ public void testMessage() throws Exception {
 		formatterService.setMessageSource(messageSource);

 		String result = formatter.format(new Obs(), Locale.ENGLISH);
-		verify(messageSource).getMessage("testing.123.testing", null, Locale.ENGLISH);


Are these really the same?

With this, I got an error. So that's why I changed the test.

[ERROR] org.openmrs.ui.framework.formatter.FormatterServiceTest.testMessage -- Time elapsed: 0.021 s <<< FAILURE! Wanted but not invoked: messageSource.getMessage( "testing.123.testing", null, en ); -> at org.openmrs.ui.framework.formatter.FormatterServiceTest.testMessage(FormatterServiceTest.java:65) Actually, there were zero interactions with this mock. at org.openmrs.ui.framework.formatter.FormatterServiceTest.testMessage(FormatterServiceTest.java:65) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:568) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:73) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:82) at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:73) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:217) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:83) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229) at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:68) at org.junit.runners.ParentRunner.run(ParentRunner.java:309) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:163) at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316) at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240) at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214) at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155) at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385) at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162) at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507) at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)

The test was verifying that this method is invoked, and with those exact parameters. Does your change do the same?

I just noticed the old test passes when run individually. It only fails when running with the other test cases. Could it be caused because I removed @DirtiesContext?

Of course you can always test and confirm your theory without first seeking permission from me. 😊

I fixed the issue.

dkayiwa · 2023-12-04T18:58:26Z

.github/PULL_REQUEST_TEMPLATE.md

+
+## Issue I worked on
+<!--- This project only accepts pull requests related to open issues -->
+<!--- Want a new feature or change? Discuss it in an issue first -->


Is this also required for the Java 17 compatibility?

I removed this

dkayiwa · 2023-12-05T11:03:47Z

api/src/test/java/org/openmrs/ui/framework/formatter/FormatterServiceTest.java

 		HandlebarsFormatterFactory classFormatter = new HandlebarsFormatterFactory();
 		classFormatter.setForClass("org.openmrs.Obs");
 		classFormatter.setTemplate("{{ message 'testing.123.testing' }} something");
-		formatterService.addClassFormatter(classFormatter);


Why did you get rid of the above?

I didn't. I just changed the variable. From formatterService to messageFormatService

It is not easy to tell what exactly changed or what was necessary to make the test pass.
For instance, why do we now instantiate the message format service like FormatterService messageFormatterService = new FormatterService()

The error I mentioned above happens because of the test leakage from the formatterService. That's why I added the method variable for it.

Can you explain more what you mean by the test leackage?

Because we removed @DirtiesContext, changes happen to the formatterService on the other two tests passed on to this test. This causes the test leakage. That's why I decided to use a separate FormatterService for this test.

Makes perfect sense! Thanks for catching that. 👍

dkayiwa · 2023-12-05T12:38:43Z

Thanks @wikumChamith 👍

dkayiwa reviewed Dec 4, 2023

View reviewed changes

wikumChamith force-pushed the ULFR-222 branch from 43830ce to 5a0ee4b Compare December 4, 2023 18:48

dkayiwa reviewed Dec 4, 2023

View reviewed changes

wikumChamith force-pushed the ULFR-222 branch 2 times, most recently from d1767d2 to a62eff0 Compare December 5, 2023 10:54

dkayiwa reviewed Dec 5, 2023

View reviewed changes

UIFR-222: Adding Java 17 Compatability

e8347ea

wikumChamith force-pushed the ULFR-222 branch from a62eff0 to e8347ea Compare December 5, 2023 11:06

dkayiwa merged commit ff12904 into openmrs:master Dec 5, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UIFR-222: Adding Java 17 Compatability #76

UIFR-222: Adding Java 17 Compatability #76

wikumChamith commented Dec 4, 2023 •

edited

Loading

dkayiwa Dec 4, 2023

wikumChamith Dec 4, 2023

dkayiwa Dec 4, 2023

dkayiwa Dec 4, 2023

wikumChamith Dec 4, 2023

dkayiwa Dec 4, 2023

wikumChamith Dec 4, 2023

dkayiwa Dec 4, 2023 •

edited

Loading

wikumChamith Dec 4, 2023

dkayiwa Dec 4, 2023

dkayiwa Dec 4, 2023

wikumChamith Dec 4, 2023

dkayiwa Dec 4, 2023 •

edited

Loading

wikumChamith Dec 5, 2023 •

edited

Loading

dkayiwa Dec 5, 2023 •

edited

Loading

wikumChamith Dec 5, 2023

dkayiwa Dec 4, 2023

wikumChamith Dec 4, 2023

dkayiwa Dec 5, 2023

wikumChamith Dec 5, 2023

dkayiwa Dec 5, 2023

wikumChamith Dec 5, 2023

dkayiwa Dec 5, 2023

wikumChamith Dec 5, 2023 •

edited

Loading

dkayiwa Dec 5, 2023

dkayiwa commented Dec 5, 2023

UIFR-222: Adding Java 17 Compatability #76

UIFR-222: Adding Java 17 Compatability #76

Conversation

wikumChamith commented Dec 4, 2023 • edited Loading

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

dkayiwa Dec 4, 2023 • edited Loading

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

dkayiwa Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

wikumChamith Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

dkayiwa Dec 5, 2023 • edited Loading

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

wikumChamith Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Dec 5, 2023

wikumChamith commented Dec 4, 2023 •

edited

Loading

dkayiwa Dec 4, 2023 •

edited

Loading

dkayiwa Dec 4, 2023 •

edited

Loading

wikumChamith Dec 5, 2023 •

edited

Loading

dkayiwa Dec 5, 2023 •

edited

Loading

wikumChamith Dec 5, 2023 •

edited

Loading