Add a JSON test suite #604

eemeli · 2024-01-16T10:59:31Z

As discussed on yesterday's call, we should bootstrap our official test suite with the one I've been putting together for the JS messageformat implementation, as it's the most comprehensive one currently available.

For the avoidance of any doubt, I am the original author of these tests and I hereby relicense my work included in this PR under the Unicode License.

I've here moved the tests originally added by @grhoten that were in test/ to test/xml/, as I wasn't sure whether we wanted to keep them as well.

…/ from messageformat/messageformat@11c95da

aphillips

Thank you! Some general observations follow.

aphillips · 2024-01-16T14:31:54Z

test/json/README.md

@@ -0,0 +1,28 @@
+The files in this directory were originally copied from the [messageformat project](https://github.com/messageformat/messageformat/tree/11c95dab2b25db8454e49ff4daadb817e1d5b770/packages/mf2-messageformat/src/__fixtures)
+and are here relicensed by their original author (Eemeli Aro) under the Unicode License.


I'm not sure if this is kosher or not. I will check with legal about the best way to handle this.

Thanks, happy to do whatever including assigning copyright. I just figure that it's best done explicitly here, given that the tests were originally Apache-2.0 licensed under the messageformat, which is an OpenJS Foundation project.

Based on our discussion with Anne (legal), as sole author you can commit this file, relying on the CLA to cover it.

aphillips · 2024-01-16T14:35:49Z

test/json/test-core.json

@@ -0,0 +1,140 @@
+[
+  { "src": "hello", "exp": "hello" },
+  { "src": "hello {world}", "exp": "hello world" },


comments would be useful in this file. It took me a second to remember that world is an unquoted.

That sounds like valuable input to our further decision-making on the format for these. Before we start applying any such changes, we should firm up the ultimate shape these will take.

aphillips · 2024-01-16T14:38:34Z

test/json/test-core.json

+    "src": "{$one} and {$two}",
+    "params": { "one": 1.3, "two": 4.2 },
+    "exp": "1.3 and 4.2"
+  },
+  {
+    "src": "{$one} et {$two}",
+    "locale": "fr",
+    "params": { "one": 1.3, "two": 4.2 },
+    "exp": "1,3 et 4,2"
+  },


These test something other than the :string formatter. It might be a good idea to move them with others of the same stripe or segregate them.

aphillips · 2024-01-16T14:40:57Z

test/json/test-core.json

+  { "src": ".local $foo = {bar} {{bar {$foo}}}", "exp": "bar bar" },
+  { "src": ".local $foo = {|bar|} {{bar {$foo}}}", "exp": "bar bar" },
+  {
+    "src": ".local $foo = {|bar|} {{bar {$foo}}}",


Might be nice to have some whitespace tests with assignments and selectors. I don't see them elsewhere in this file?

aphillips · 2024-01-16T14:41:46Z

test/json/test-core.json

+    ]
+  },
+  {
+    "src": "{#tag a:foo=|foo| b:bar=$bar}",


Missing standalone tags?

test/json/test-core.json.d.ts

aphillips · 2024-01-16T14:44:17Z

test/json/test-core.json.d.ts

+export type TestMessage = {
+  /** The MF2 syntax source. */
+  src: string;
+  /** The locale to use for formatting. Defaults to 'en-US'. */


Did you consider defaulting to und? The root locale is US English-like but also somewhat invariant.

Alas, JS doesn't support an und locale.

It works in Chrome. It also works in FF, except that the locale's name data isn't present.

It only looks like it works, though. If you try this, for instance, you'll get your default locale (probably en-US) rather than und:

new Intl.NumberFormat('und').resolvedOptions().locale

aphillips · 2024-01-16T14:44:40Z

test/json/test-core.json.d.ts

+  /** The locale to use for formatting. Defaults to 'en-US'. */
+  locale?: string;
+  /** Parameters to pass in to the formatter for resolving external variables. */
+  params?: Record<string, string | number | null | undefined>;


include date type here?

Nope, because this is JSON data which doesn't inherently support dates.

aphillips · 2024-01-16T14:47:12Z

test/json/test-functions.json

+    { "src": "hello {-4.20 :integer}", "exp": "hello -4" },
+    { "src": "hello {0.42e+1 :integer}", "exp": "hello 4" },
+    {
+      "src": ".match {$foo :integer} one {{one}} * {{other}}",


when writing plural tests, I often find it a good idea to build out the variant matrix. Yes, en-US (or und for that matter) won't touch the two or few or whatever keyword. But it can help catch errors if one of the other keywords triggers incorrectly. It also allows the test itself to be recycled for other locales.

aphillips · 2024-01-16T14:49:45Z

test/json/test-functions.json

+      "exp": "=1.2"
+    }
+  ],
+  "number": [


I would move :number ahead of :integer. Note that you don't test any of :integer's options, e.g. minimumIntegerDigits, signDisplay, et al.

Co-authored-by: Addison Phillips <[email protected]>

eemeli · 2024-01-16T16:23:59Z

@aphillips I would very much prefer to not touch the data files at all in this PR, even though as you've pointed out there are a few cases where the tests are e.g. incorrectly categorised, and definitely incomplete.

As we squash our commits, that will allow this one PR to be the place where we adopt the messageformat test suite and for which we need to make sure that any copyright assignment or licensing is done right. Then subsequent PRs on top of this will be done wholly under the Unicode license.

aphillips

Per your comment, we can commit this without addressing my comments.

It would make sense to do a more thorough job of it, though. Should we work on these files or something else?

eemeli · 2024-01-18T22:05:04Z

Should we work on these files or something else?

My recommendation would be to get these in, and then start iterating on them. Starting from a copy has the benefit of having at least one implementation for which all of these tests pass, which makes it easier to validate any changes.

grhoten · 2024-01-20T00:32:19Z

I've here moved the tests originally added by @grhoten that were in test/ to test/xml/, as I wasn't sure whether we wanted to keep them as well.

I say delete them. They don't fit well into the current design of MF2, and the files can always be resurrected from the history if necessary.

grhoten · 2024-01-20T00:38:52Z

test/json/test-functions.json

+  "ordinal": [
+    {
+      "src": ".match {$foo :ordinal} one {{st}} two {{nd}} few {{rd}} * {{th}}",
+      "params": { "foo": 1 },
+      "exp": "st"
+    },
+    {
+      "src": ".match {$foo :ordinal} one {{st}} two {{nd}} few {{rd}} * {{th}}",
+      "params": { "foo": 2 },
+      "exp": "nd"
+    },
+    {
+      "src": ".match {$foo :ordinal} one {{st}} two {{nd}} few {{rd}} * {{th}}",
+      "params": { "foo": 3 },
+      "exp": "rd"
+    },
+    {
+      "src": ".match {$foo :ordinal} one {{st}} two {{nd}} few {{rd}} * {{th}}",
+      "params": { "foo": 4 },
+      "exp": "th"
+    },


Do you have the ability to reference ICU RBNF functionality? ICU MessageFormat could reference them as the spellout and ordinal, which conflicts with this plural rule ordinal.

See here for reference: https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/classMessageFormat.html

Please ignore duration from MessageFormat. It's deprecated, and you need a decent message formatting framework to do it right.

You're missing the point of this test, which is testing the ordinal selector (== selectordinal in MF1). @eemeli has used the English ordinal suffixes in the test for readability in a test that is certainly not ready for any locale other than en-US.

Note well that we are considering spellout, ordinal, and duration for inclusion into the default registry as formatters--in another set of issues and design documents. I recommended against including spellout and duration in the default registry. ordinal as a formatter is on the bubble (only ICU4J has it, not Intl or ICU4X) and there are obvious functionality problems with ordinals in the current implementation.

eemeli · 2024-01-22T21:03:42Z

@aphillips Applied the updates discussed on the call; this should now be good to merge.

eemeli added 3 commits January 16, 2024 11:38

Move test/* -> test/xml/

bf78a35

Add test/json/ as a copy of packages/mf2-messageformat/src/__fixtures…

c65b5cd

…/ from messageformat/messageformat@11c95da

Add test/json/README.md

534c4a0

eemeli added requirements Issues related with MF requirements list LDML45 LDML45 Release (Tech Preview) labels Jan 16, 2024

aphillips added the test-suite Issue pertains to tests label Jan 16, 2024

aphillips reviewed Jan 16, 2024

View reviewed changes

Apply suggestions from code review

79fac1f

Co-authored-by: Addison Phillips <[email protected]>

aphillips approved these changes Jan 18, 2024

View reviewed changes

grhoten reviewed Jan 20, 2024

View reviewed changes

Drop the previous XML tests, move test/json/* to just test/

54411ce

aphillips merged commit d06033b into main Jan 22, 2024

aphillips deleted the json-tests branch January 22, 2024 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a JSON test suite #604

Add a JSON test suite #604

eemeli commented Jan 16, 2024

aphillips left a comment

aphillips Jan 16, 2024

eemeli Jan 16, 2024

aphillips Jan 18, 2024

aphillips Jan 16, 2024

eemeli Jan 16, 2024

aphillips Jan 16, 2024

aphillips Jan 16, 2024

aphillips Jan 16, 2024

aphillips Jan 16, 2024

eemeli Jan 16, 2024

aphillips Jan 18, 2024

eemeli Jan 18, 2024

aphillips Jan 16, 2024

eemeli Jan 16, 2024

aphillips Jan 16, 2024

aphillips Jan 16, 2024

eemeli commented Jan 16, 2024

aphillips left a comment

eemeli commented Jan 18, 2024

grhoten commented Jan 20, 2024

grhoten Jan 20, 2024

aphillips Jan 20, 2024

eemeli commented Jan 22, 2024

		@@ -0,0 +1,28 @@
		The files in this directory were originally copied from the [messageformat project](https://github.com/messageformat/messageformat/tree/11c95dab2b25db8454e49ff4daadb817e1d5b770/packages/mf2-messageformat/src/__fixtures)
		and are here relicensed by their original author (Eemeli Aro) under the Unicode License.

Add a JSON test suite #604

Add a JSON test suite #604

Conversation

eemeli commented Jan 16, 2024

aphillips left a 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

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

eemeli commented Jan 16, 2024

aphillips left a comment

Choose a reason for hiding this comment

eemeli commented Jan 18, 2024

grhoten commented Jan 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemeli commented Jan 22, 2024