Skip to content

Make expErrors use an array (and only an array) #1076

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.

Sign up for GitHub

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

Merged
aphillips merged 12 commits into from
Jul 7, 2025
Merged

Make expErrors use an array (and only an array) #1076

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3f11bcb
Issue 993: make expErrors only
mihnita Jun 2, 2025
51a2a4b
Adding `expErrors` values for fallback.json
mihnita Jun 8, 2025
5db6c5d
Update test/README.md
mihnita Jun 8, 2025
7cde5a3
Update test/README.md
mihnita Jun 8, 2025
679ec61
Update test/tests/fallback.json
mihnita Jun 8, 2025
989bdc0
Update test/tests/fallback.json
mihnita Jun 8, 2025
542e6ec
Update test/tests/fallback.json
mihnita Jun 8, 2025
c5f71d3
Update test/tests/fallback.json
mihnita Jun 8, 2025
208f524
Fix 'Bad Operand' to 'Bad Option'
mihnita Jun 30, 2025
ae7b8d7
Update test/README.md
mihnita Jul 7, 2025
08b1cbf
Fix test case with proper error code
mihnita Jul 7, 2025
c43fd62
Fix test case with proper error code
mihnita Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions test/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ the behaviour of calling it as the `rv` value of MatchSelectorKeys(`rv`, `keys`)
depends on its `Input`, `DecimalPlaces` and `FailsSelect` values.

- If `FailsSelect` is `true`,
calling the method will emit a _Message Function Error_
calling the method will emit a _Bad Option_ error
and not return any value.
- If the `Input` is 1 and `DecimalPlaces` is 1,
the method will return some slice of the list « `'1.0'`, `'1'` »,
Expand Down Expand Up @@ -185,7 +185,7 @@ rather than being concatenated into a single string.

If `FailsFormat` is `true`,
attempting to format the _placeholder_ to any formatting target will
emit a _Message Function Error_.
emit a _Bad Option_ error.

### `:test:select`

Expand Down
5 changes: 1 addition & 4 deletions test/schemas/v0/tests.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,10 +382,7 @@
},
"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"
],
"type": "array",
"items": {
"type": "object",
"additionalProperties": false,
Expand Down
27 changes: 17 additions & 10 deletions test/tests/fallback.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,52 +4,59 @@
"description": "Test cases for fallback behaviour.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US",
"expErrors": true
"locale": "en-US"
},
"tests": [
{
"description": "function with unquoted literal operand",
"src": "{42 :test:function fails=format}",
"exp": "{|42|}",
"expParts": [{ "type": "fallback", "source": "|42|" }]
"expParts": [{ "type": "fallback", "source": "|42|" }],
"expErrors": [{ "type": "bad-operand" }]
Copy link
Collaborator

@eemeli eemeli Jul 1, 2025

Choose a reason for hiding this comment

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

Suggested change
"expErrors": [{ "type": "bad-operand" }]
"expErrors": [{ "type": "bad-option" }]

Copy link
Member

@aphillips aphillips Jul 2, 2025

Choose a reason for hiding this comment

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

Hmm... I found this confusing until I remembered that we agreed to use bad-option to avoid creating a label for message-function-error in the not at all official definition under tests here. Non-WG members won't have the benefit of having been on the telecon when we discussed it 😉

Unlike the other classes of error, MFE can have implementation-defined errors. We should make this clearer in the test suite text so that implementations do not merely ape our selection of bad-option when an internal exception/error occurs. I think it would be okay to commit this, but we need to put more effort into the text in the /tests page to make super clear that bad-option is not required by non-test functions.

Copy link
Collaborator

@eemeli eemeli Jul 2, 2025

Choose a reason for hiding this comment

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

Agreed on all counts. I also noticed while implementing this change that we lack a generic error for failures in formatting or selection, which could be due to the operand, options, or the phase of the moon. As in, values for which resolution succeeds, but then the MessageValue.formatToString() or similar call throws an error. It's pretty likely to involve some bad option, but that's not actually guaranteed -- and beyond the scope of this PR.

Copy link
Collaborator Author

@mihnita mihnita Jul 7, 2025

Choose a reason for hiding this comment

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

Done.

},
{
"description": "function with quoted literal operand",
"src": "{|C:\\\\| :test:function fails=format}",
"exp": "{|C:\\\\|}"
"exp": "{|C:\\\\|}",
"expErrors": [{ "type": "bad-operand" }]
},
{
"description": "unannotated implicit input variable",
"src": "{$var}",
"exp": "{$var}"
"exp": "{$var}",
"expErrors": [{ "type": "unresolved-variable" }]
},
{
"description": "annotated implicit input variable",
"src": "{$var :number}",
"exp": "{$var}",
"expParts": [{ "type": "fallback", "source": "$var" }]
"expParts": [{ "type": "fallback", "source": "$var" }],
"expErrors": [{ "type": "unresolved-variable" }, { "type": "bad-operand" }]
},
{
"description": "local variable with unknown function in declaration",
"src": ".local $var = {|val| :test:undefined} {{{$var}}}",
"exp": "{$var}"
"exp": "{$var}",
"expErrors": [{ "type": "unknown-function" }]
},
{
"description": "function with local variable operand with unknown function in declaration",
"src": ".local $var = {|val| :test:undefined} {{{$var :test:function}}}",
"exp": "{$var}"
"exp": "{$var}",
"expErrors": [{ "type": "unknown-function" }, { "type": "bad-operand" }]
},
{
"description": "local variable with unknown function in placeholder",
"src": ".local $var = {|val|} {{{$var :test:undefined}}}",
"exp": "{$var}"
"exp": "{$var}",
"expErrors": [{ "type": "unknown-function" }]
},
{
"description": "function with no operand",
"src": "{:test:undefined}",
"exp": "{:test:undefined}",
"expParts": [{ "type": "fallback", "source": ":test:undefined" }]
"expParts": [{ "type": "fallback", "source": ":test:undefined" }],
"expErrors": [{ "type": "unknown-function" }]
}
]
}
24 changes: 9 additions & 15 deletions test/tests/functions/currency.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
"description": "The built-in formatter and selector for currencies.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US"
"locale": "en-US",
"expErrors": []
},
"tests": [
{
Expand All @@ -24,33 +25,26 @@
"expErrors": [{ "type": "bad-operand" }]
},
{
"src": "{42 :currency currency=EUR}",
"expErrors": false
"src": "{42 :currency currency=EUR}"
},
{
"src": ".local $n = {42 :number} {{{$n :currency currency=EUR}}}",
"expErrors": false
"src": ".local $n = {42 :number} {{{$n :currency currency=EUR}}}"
},
{
"src": ".local $n = {42 :integer} {{{$n :currency currency=EUR}}}",
"expErrors": false
"src": ".local $n = {42 :integer} {{{$n :currency currency=EUR}}}"
},
{
"src": ".local $n = {42 :currency currency=EUR} {{{$n :currency}}}",
"expErrors": false
"src": ".local $n = {42 :currency currency=EUR} {{{$n :currency}}}"
},
{
"src": "{42 :currency currency=EUR fractionDigits=auto}",
"expErrors": false
"src": "{42 :currency currency=EUR fractionDigits=auto}"
},
{
"src": "{42 :currency currency=EUR fractionDigits=2}",
"expErrors": false
"src": "{42 :currency currency=EUR fractionDigits=2}"
},
{
"src": "{$x :currency currency=EUR}",
"params": [{ "name": "x", "value": 41 }],
"expErrors": false
"params": [{ "name": "x", "value": 41 }]
},
{
"src": ".local $n = {42 :currency currency=EUR} .match $n * {{other}}",
Expand Down
2 changes: 1 addition & 1 deletion test/tests/functions/date.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US",
"expErrors": false
"expErrors": []
},
"tests": [
{
Expand Down
2 changes: 1 addition & 1 deletion test/tests/functions/datetime.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US",
"expErrors": false
"expErrors": []
Copy link
Member

@aphillips aphillips Jun 2, 2025

Choose a reason for hiding this comment

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

In fallback.md you left out expErrors. Here you supply an empty array. I think we should probably omit this one?

Copy link
Collaborator

@mradbourne mradbourne Jun 5, 2025

Choose a reason for hiding this comment

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

@aphillips It's required in this file otherwise some tests would be left without an expectation (e.g. lines 44-55). This would also be the case in time.json.

As Eemeli mentioned in his review comment, I think we need to keep the expErrors in fallback.md.

Copy link
Collaborator Author

@mihnita mihnita Jun 8, 2025

Choose a reason for hiding this comment

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

I think we are talking about fallback.json, not fallback.md?

I updated fallback.json and added errors for each individual case, instead of doing it in defaultTestProperties

Copy link
Collaborator Author

@mihnita mihnita Jun 8, 2025

Choose a reason for hiding this comment

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

For date.json, datetime.json and time.json I added "expErrors": [] to defaultTestProperties because the schema says that entries are required to have one of exp, expParts or expErrors

In these 3 files we had tests with src only.
So I had to add a expErrors (not enough info for exp, expParts, exact date/time formatting not part of the spec)

},
"tests": [
{
Expand Down
2 changes: 1 addition & 1 deletion test/tests/functions/time.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US",
"expErrors": false
"expErrors": []
},
"tests": [
{
Expand Down