fix: signDisplay plus sign behaviour fixed for NumberFormat (#1483)

Summary: Fixes following issue: - #1466 - #1138 - #789 **Why this change is made?** There are few cases where signDisplay is not being handled correctly. Examples: ``` 1. new Intl.NumberFormat('de-DE', { style: 'currency', currency: 'EUR', signDisplay: 'exceptZero', }).format(8537.71) Output: +8,537,71+ Expected: +8.537,71 € 2. new Intl.NumberFormat('ja-JP', { style: 'currency', currency: 'JPY', signDisplay: 'exceptZero' }).format(123456.789) Output: +123,457 Expected: +￥123,457 ``` **ChangeLog** This PR updates the implementation and testing of the signDisplay functionality in the DecimalFormat class within the Hermes engine, specifically for Android API level 31 and above. **Key Changes:** Implementation: - Integrated the **setSignAlwaysShown** method of DecimalFormat for API level 31 and above to control the display of the sign (+ or -) based on the signDisplay option. [For more detail about setSignAlwaysShown check [here](https://developer.android.com/reference/android/icu/text/DecimalFormat#setSignAlwaysShown(boolean))] - For API levels below 31, maintained the existing logic for handling sign display, ensuring backward compatibility. Pull Request resolved: #1483 Test Plan: - Added a comprehensive set of test cases in **HermesIntlAndroidTest.java** to validate the behaviour of the signDisplay functionality, specifically for API level 31 and above. - Test cases cover scenarios for signDisplay: `NEVER | ALWAYS | EXCEPTZERO` Reviewed By: avp Differential Revision: D62153166 Pulled By: neildhar fbshipit-source-id: d45c55ae7ffbfbc38ec4b331339859b8c96486c7
facebook · Sep 17, 2024 · a75e14a · a75e14a
1 parent 2ca5da2
commit a75e14a
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 31 deletions.
diff --git a/android/build.gradle b/android/build.gradle
@@ -39,7 +39,7 @@ buildscript {
     System.getenv("HOME") + "/fbsource"
 
     minSdkVersion = 16
-    compileSdkVersion = 28
+    compileSdkVersion = 31
     abis = project.hasProperty('abis') ?
          project.getProperty("abis").split(",") :
          ["arm64-v8a", "armeabi-v7a", "x86_64", "x86"]

diff --git a/android/intltest/build.gradle b/android/intltest/build.gradle
@@ -56,7 +56,7 @@ task prepareTests() {
 
 // TODO: Figure out how to deduplicate this file and intl/build.gradle
 android {
-  compileSdkVersion 24
+  compileSdkVersion 31
 
   defaultConfig {
     minSdkVersion 16

diff --git a/android/intltest/java/com/facebook/hermes/test/HermesInstrumentationTest.java b/android/intltest/java/com/facebook/hermes/test/HermesInstrumentationTest.java
@@ -9,12 +9,14 @@
 
 import static org.assertj.core.api.Java6Assertions.assertThat;
 
-import android.test.InstrumentationTestCase;
+import androidx.test.runner.AndroidJUnit4;
 import java.util.Locale;
 import java.util.TimeZone;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 
-public class HermesInstrumentationTest extends InstrumentationTestCase {
+@RunWith(AndroidJUnit4.class)
+public class HermesInstrumentationTest {
 
   @Test
   public void testEvaluatingJavaScript() {
@@ -143,6 +145,6 @@ public void testGetHermesEpilogue() {
     byte[] epilogue =
         HermesEpilogue.getHermesBytecodeMetadata(
             HermesEpilogueTestData.getBytecodeWithEpilogue(EXPECTED_EPILOGUE));
-    assertEquals(EXPECTED_EPILOGUE, new String(epilogue));
+    assertThat(new String(epilogue)).isEqualTo(EXPECTED_EPILOGUE);
   }
 }
diff --git a/android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java b/android/intltest/java/com/facebook/hermes/test/HermesIntlAndroidTest.java
@@ -10,18 +10,24 @@
 import static org.assertj.core.api.Java6Assertions.assertThat;
 
 import android.content.res.AssetManager;
-import android.test.InstrumentationTestCase;
+import android.os.Build;
+import androidx.test.platform.app.InstrumentationRegistry;
+import androidx.test.runner.AndroidJUnit4;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.util.stream.Collectors;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+public class HermesIntlAndroidTest {
 
-public class HermesIntlAndroidTest extends InstrumentationTestCase {
   @Test
   public void testIntlFromAsset() throws IOException {
-    AssetManager assets = getInstrumentation().getContext().getAssets();
+    AssetManager assets =
+        InstrumentationRegistry.getInstrumentation().getTargetContext().getAssets();
     InputStream is = assets.open("intl.js");
     String script =
         new BufferedReader(new InputStreamReader(is)).lines().collect(Collectors.joining("\n"));
@@ -32,7 +38,8 @@ public void testIntlFromAsset() throws IOException {
 
   @Test
   public void testNumberFormatFractionDigitsFromAsset() throws IOException {
-    AssetManager assets = getInstrumentation().getContext().getAssets();
+    AssetManager assets =
+        InstrumentationRegistry.getInstrumentation().getTargetContext().getAssets();
     InputStream is = assets.open("number-format-fraction-digits.js");
     String script =
         new BufferedReader(new InputStreamReader(is)).lines().collect(Collectors.joining("\n"));
@@ -80,4 +87,49 @@ public void testDateTimeFormatCaseInsensitivity() {
       assertThat(result).isEqualTo("9/24, 6:00 PM");
     }
   }
+
+  @Test
+  public void testSignDisplayAlwaysForApiLevelAbove31() {
+    if (Build.VERSION.SDK_INT >= 31) {
+      try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
+        rt.evaluateJavaScript(
+            "var nf = new Intl.NumberFormat('de-DE', { signDisplay: 'always', currency: 'EUR',"
+                + " style: 'currency' });\n"
+                + "var result = nf.format(8537.71);");
+
+        String result = rt.getGlobalStringProperty("result");
+        assertThat(result).isEqualTo("+8.537,71 €");
+      }
+    }
+  }
+
+  @Test
+  public void testSignDisplayNeverForApiLevelAbove31() {
+    if (Build.VERSION.SDK_INT >= 31) {
+      try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
+        rt.evaluateJavaScript(
+            "var nf = new Intl.NumberFormat('de-DE', { signDisplay: 'never', currency: 'EUR',"
+                + " style: 'currency' });\n"
+                + "var result = nf.format(8537.71);");
+
+        String result = rt.getGlobalStringProperty("result");
+        assertThat(result).isEqualTo("8.537,71 €");
+      }
+    }
+  }
+
+  @Test
+  public void testSignDisplayExceptZeroForApiLevelAbove31() {
+    if (Build.VERSION.SDK_INT >= 31) {
+      try (JSRuntime rt = JSRuntime.makeHermesRuntime()) {
+        rt.evaluateJavaScript(
+            "var nf = new Intl.NumberFormat('de-DE', { signDisplay: 'exceptZero', currency: 'EUR',"
+                + " style: 'currency'});\n"
+                + "var result = nf.format(8537.71);");
+
+        String result = rt.getGlobalStringProperty("result");
+        assertThat(result).isEqualTo("+8.537,71 €");
+      }
+    }
+  }
 }
diff --git a/android/intltest/java/com/facebook/hermes/test/HermesIntlTest262.java b/android/intltest/java/com/facebook/hermes/test/HermesIntlTest262.java
@@ -8,9 +8,10 @@
 import static org.assertj.core.api.Java6Assertions.assertThat;
 
 import android.content.res.AssetManager;
-import android.test.InstrumentationTestCase;
 import android.text.TextUtils;
 import android.util.Log;
+import androidx.test.platform.app.InstrumentationRegistry;
+import androidx.test.runner.AndroidJUnit4;
 import com.facebook.hermes.test.JSRuntime;
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -23,15 +24,19 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.Stack;
+import org.junit.Test;
+import org.junit.runner.RunWith;
 
 // Run "./gradlew :intltest:prepareTests" from the root to copy the test files to the
 // APK assets.
-public class HermesIntlTest262 extends InstrumentationTestCase {
+@RunWith(AndroidJUnit4.class)
+public class HermesIntlTest262 {
 
   private static final String LOG_TAG = "HermesIntlTest";
 
   protected void evalScriptFromAsset(JSRuntime rt, String filename) throws IOException {
-    AssetManager assets = getInstrumentation().getContext().getAssets();
+    AssetManager assets =
+        InstrumentationRegistry.getInstrumentation().getTargetContext().getAssets();
     InputStream is = assets.open(filename);
 
     BufferedReader r = new BufferedReader(new InputStreamReader(is));
@@ -56,11 +61,13 @@ protected void evaluateCommonScriptsFromAsset(JSRuntime rt) throws IOException {
     evalScriptFromAsset(rt, "test262/harness/testTypedArray.js");
   }
 
+  @Test
   public void test262Intl() throws IOException {
     Set<String> skipList = getSkipList();
     Stack<String> testFiles = new Stack<>();
     testFiles.push("test262/test");
-    AssetManager assets = getInstrumentation().getContext().getAssets();
+    AssetManager assets =
+        InstrumentationRegistry.getInstrumentation().getTargetContext().getAssets();
     ArrayList<String> ranTests = new ArrayList<>();
     HashMap<String, String> failedTests = new HashMap<>();
 

diff --git a/lib/Platform/Intl/java/com/facebook/hermes/intl/PlatformNumberFormatterICU.java b/lib/Platform/Intl/java/com/facebook/hermes/intl/PlatformNumberFormatterICU.java
@@ -155,24 +155,35 @@ public PlatformNumberFormatterICU setSignDisplay(
       android.icu.text.DecimalFormat decimalFormat = (android.icu.text.DecimalFormat) mNumberFormat;
       android.icu.text.DecimalFormatSymbols symbols = decimalFormat.getDecimalFormatSymbols();
 
-      switch (signDisplay) {
-        case NEVER:
-          decimalFormat.setPositivePrefix("");
-          decimalFormat.setPositiveSuffix("");
-
-          decimalFormat.setNegativePrefix("");
-          decimalFormat.setNegativeSuffix("");
-
-          break;
-        case ALWAYS:
-        case EXCEPTZERO:
-          if (!decimalFormat.getNegativePrefix().isEmpty())
-            decimalFormat.setPositivePrefix(new String(new char[] {symbols.getPlusSign()}));
-
-          if (!decimalFormat.getNegativeSuffix().isEmpty())
-            decimalFormat.setPositiveSuffix(new String(new char[] {symbols.getPlusSign()}));
-
-          break;
+      if (Build.VERSION.SDK_INT >= 31) {
+        switch (signDisplay) {
+          case NEVER:
+            decimalFormat.setSignAlwaysShown(false);
+            break;
+          case ALWAYS:
+          case EXCEPTZERO:
+            decimalFormat.setSignAlwaysShown(true);
+            break;
+        }
+      } else {
+        switch (signDisplay) {
+          case NEVER:
+            decimalFormat.setPositivePrefix("");
+            decimalFormat.setPositiveSuffix("");
+
+            decimalFormat.setNegativePrefix("");
+            decimalFormat.setNegativeSuffix("");
+
+            break;
+          case ALWAYS:
+          case EXCEPTZERO:
+            if (!decimalFormat.getNegativePrefix().isEmpty())
+              decimalFormat.setPositivePrefix(new String(new char[] {symbols.getPlusSign()}));
+
+            if (!decimalFormat.getNegativeSuffix().isEmpty())
+              decimalFormat.setPositiveSuffix(new String(new char[] {symbols.getPlusSign()}));
+            break;
+        }
       }
     }