Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new test schema #767

Merged
merged 25 commits into from
Apr 29, 2024
Merged

Add new test schema #767

merged 25 commits into from
Apr 29, 2024

Conversation

mradbourne
Copy link
Collaborator

This PR adds a schema for the JSON test files and refactors the existing test files to match the schema.

The schema is designed for easy interoperability with the unicode/conformance test suite. This is not a mandatory requirement but a nice-to-have.

Most of the content removed from the README is now included within schema.json.

The format of params has changed. They now use a { type, name, value } structure instead of { name: value }. Although instances of { "type": "string", "name": "foo", "value": "string-value" } may seem unnecessary, this pattern allows us to specify params that are not part of the JSON spec (e.g. { "type": "datetime", "name": "foo", "value": "2006-01-02T15:04:06" }.

The TypeScript .d.ts files have been removed for now because they no longer match the JSON content. I'd be happy to re-add them if/when needed.

There is a Visual Studio Code settings file included. Although contributors use many editors and IDEs, I thought it would be useful to provide at least one editor/schema integration out-of-the-box.

@aphillips aphillips added normative test-suite LDML46 LDML46 Release (Tech Preview - October 2024) labels Apr 12, 2024
@aphillips aphillips added the Agenda+ Requested for upcoming teleconference label Apr 13, 2024
Copy link
Collaborator

@eemeli eemeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good! Some minor-ish concerns mentioned inline.

We should also retain a human-friendly description of the test files in the README.

test/tests/functions/ordinal.json Outdated Show resolved Hide resolved
test/tests/data-model-errors.json Outdated Show resolved Hide resolved
test/schema.json Outdated Show resolved Hide resolved
test/schema.json Outdated Show resolved Hide resolved
test/schema.json Outdated Show resolved Hide resolved
test/schema.json Outdated Show resolved Hide resolved
test/schema.json Outdated Show resolved Hide resolved
test/schema.json Outdated Show resolved Hide resolved
test/schema.json Outdated Show resolved Hide resolved
@mihnita
Copy link
Collaborator

mihnita commented Apr 14, 2024

Some comments about the overall structure (we can fine tune later):

1. I found the ability to group tests together in the same file pretty handy before (the way data-model-errors.json and test-functions.json used to be

So instead of one hard-coded "tests" array, can we make that a map, where the key can be anything, not just "test"?

And with "defaultTestParams" at group level.


2. In my implementation of ICU4J MF2, when trying to use the existing .json files, I found that some source strings are extremely long, unreadable.

It would be handy (and something I did in ICU) to allow an array of source(s) as an alternative to "src"

Compare:

"src": ".input {$exp :datetime timeStyle=short}\n.input {$user :string}\n.local $longExp = {$exp :datetime dateStyle=long}\n.local $zooExp = {$exp :datetime dateStyle=short timeStyle=$tsOver}\n{{Hello John, you want '{$exp}', '{$longExp}', or '{$zooExp}' or even '{$exp :datetime dateStyle=full}'?}}",

to:

"srcs": [
    ".input {$exp :datetime timeStyle=short}\n",
    ".input {$user :string}\n",
    ".local $longExp = {$exp :datetime dateStyle=long}\n",
    ".local $zooExp = {$exp :datetime dateStyle=short timeStyle=$tsOver}\n",
    "{{Hello John, you want '{$exp}', '{$longExp}', or '{$zooExp}' or even '{$exp :datetime dateStyle=full}'?}}"
],

3. When testing the results would be good to be able to specify expected results in a more flexible way

That is to accommodate for different results between implementations, while still testing that the "plumbing" works and that we really format to a date / time / whatever.

Things like "one of": "expOneOf": [ "9:30 PM", "9:30\u202FPM" ] Or reg-exp: : "expRegExp": [ "9:30.[Pp].[Mm]"]


4. Allowing for an optional "comment" or "description" in each test

test/tests/core.json Outdated Show resolved Hide resolved
@aphillips aphillips removed the Agenda+ Requested for upcoming teleconference label Apr 15, 2024
@mradbourne
Copy link
Collaborator Author

@mihnita Thanks for the comments! I'm replying here to capture our conversation from the WG meeting:

1. I found the ability to group tests together in the same file pretty handy before (the way data-model-errors.json and test-functions.json used to be

As this is a non-breaking change, we'll keep it in mind as we write tests and propose it in a separate PR if needed.

My only concern was that having multiple defaultTestParams properties might make the test inputs more difficult to establish at a glance. The trade-off is less flexibility in the test file though. Lets revisit with some example tests that need that flexibility. I've added a description property to each test that might help with grouping (albeit not nesting) related tests.

2. In my implementation of ICU4J MF2, when trying to use the existing .json files, I found that some source strings are extremely long, unreadable.

It would be handy (and something I did in ICU) to allow an array of source(s) as an alternative to "src"

Done 👍

3. When testing the results would be good to be able to specify expected results in a more flexible way

As with point 1, we can discuss this non-breaking change up later. The first set of spec tests should be nice and deterministic. The oneOf would be for tests where results vary by implementation.

4. Allowing for an optional "comment" or "description" in each test

Done 👍 . Thanks!

Copy link
Collaborator

@eemeli eemeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did another review pass, a couple more questions inline.

Additionally, I think we need a way to mark inputs that are:

  1. Not well formed, i.e. syntax errors.
  2. Not valid, i.e. data model errors.

Maybe a new expInvalid: 'syntax' | 'datamodel' option, or some special values for expErrors?

test/schemas/tests-schema-0.0.1.json Outdated Show resolved Hide resolved
test/schemas/tests-schema-0.0.1.json Outdated Show resolved Hide resolved
test/schemas/v0-0-1/tests.schema.json Outdated Show resolved Hide resolved
test/schemas/v0-0-1/tests.schema.json Outdated Show resolved Hide resolved
@echeran
Copy link
Collaborator

echeran commented Apr 29, 2024

The PR looks good. I want to add automated guarantees that make use of this PR's schema to ensure that our test files are well-formed JSON and adhere to the schema. So I created mradbourne#1

That PR is in @mradbourne 's personal fork with a destination of this PR. PTAL before merging.

Comment on lines 290 to 295
"not": {
"required": [
"parts",
"value"
]
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is "not" : { "required": [ .... ] } equivalent to "additionalProperties": true ? or are you asserting that these fields should never be considered required, as a statement for now & in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is asserting that parts and value are not both defined at the same time.

@eemeli
Copy link
Collaborator

eemeli commented Apr 29, 2024

I think I'd prefer a schema directory like test/schemas/v0/ (with integer versions) or test/schemas/v0.0.1/ instead of test/schemas/v0-0-1/, but that's a pretty minor quibble and can be addressed later.

@mradbourne
Copy link
Collaborator Author

@echeran Thanks for raising mradbourne#1
I'd like to do this as a separate PR to unicode-org/message-format-wg as it raises some new questions/decisions.

@mradbourne mradbourne merged commit ed6b1da into unicode-org:main Apr 29, 2024
1 check passed
mradbourne added a commit to mradbourne/message-format-wg that referenced this pull request Apr 29, 2024
@mradbourne mradbourne deleted the test-schema branch July 22, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LDML46 LDML46 Release (Tech Preview - October 2024) normative test-suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants