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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 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
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 Operand_ 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 Operand_ 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was dropping this intentional? All of the messages in this file are expected to experience fallback and emit at least one error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added what I though to be the most appropriate errors for each case.

For :test:function fails=format the test/README.md is not specific enough, it only says "will emit a Message Function Error".

So from all the message function errors I chose the one that I though might be the best fit, Bad Operand, and I also updated the README.md file with it.

I did that for both fails=format and fails=select.

"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" }]
},
{
"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

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

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

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

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": []
Copy link
Member

Choose a reason for hiding this comment

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

ibid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see above

},
"tests": [
{
Expand Down