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

Update the test schema to make expErrors "array" only #993

Open
mihnita opened this issue Jan 30, 2025 · 3 comments
Open

Update the test schema to make expErrors "array" only #993

mihnita opened this issue Jan 30, 2025 · 3 comments
Labels
LDML47 LDML 47 Release (Stable) test-suite

Comments

@mihnita
Copy link
Collaborator

mihnita commented Jan 30, 2025

Right now that is an array or a boolean:

"expErrors": {
  "description": "The runtime errors expected to be emitted when formatting the message. If expErrors is either absent or empty, the message must be formatted without errors.",
  "type": [
    "array",
    "boolean"
  ],
  ...

Why:

  1. Something that can be one of two different types makes it harder to consume the json from statically typed languages
  2. It is not useful in any way.
    The description already says "If expErrors is either absent or empty, the message must be formatted without errors". That covers the possible expErrors = false.
    So we already have 2 ways to say "nothing fails", we don't need 3 ways.
    And if we expect an error, then we should say what that error is, not just say "yes, we expect errors" (expErrors = true).

The only use of expErrors as boolean right now is in test/tests/functions/currency.json, and all instances are "expErrors": false.
Which is not needed (as per description missing expErrors means no errors).
And that is what we do in all other tests, we don't specify expErrors when none are expected.

mihnita added a commit to mihnita/message-format-wg that referenced this issue Jan 30, 2025
mihnita added a commit to mihnita/message-format-wg that referenced this issue Jan 30, 2025
mihnita added a commit to mihnita/message-format-wg that referenced this issue Jan 30, 2025
@mihnita
Copy link
Collaborator Author

mihnita commented Jan 30, 2025

I am wrong, there are several files with expErrors boolean, not just currency.json

But all use expErrors = false (which can very well be missing, or an empty array).

The one exception with expErrors = true is test/tests/fallback.json

Which I think is cleaner if we specify why the failure, not just true.

@aphillips aphillips added test-suite LDML47 LDML 47 Release (Stable) labels Jan 31, 2025
@mradbourne
Copy link
Collaborator

I support this. I think we have enough tests now to say that we're not getting much value from { expErrors: true } and may as well simplify 👍

@mihnita
Copy link
Collaborator Author

mihnita commented Feb 4, 2025

If we are into simplifying this area, at this point I think we can also make the expected errors simple strings.
Right now they are objects with a single key, always "type".

So instead of:

"expErrors": [{ "type": "unknown-function" }, { "type": "bad-selector" }]

we can say:

"expErrors": [ "unknown-function", "bad-selector" ]

Less verbose, and the exact same info.

It does not mean that implementations are forced to represent the errors as strings instead of being typed.
It only means that the test suites might have to "massage" the direct result of the parsing,
or at the point where it compares the real error (an object) with the expected on from json (a string).

We already forced C/C++/Java to do all kind of "massaging" of the data loaded from json.

It would be easy for an implementation to change a test from:

// Compare two Error objects
if (realError == expectedError) ...

to:

// Compare two strings
if (realError.type == expectedError) ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LDML47 LDML 47 Release (Stable) test-suite
Projects
None yet
Development

No branches or pull requests

3 participants