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.

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

Jump to bottom

Add tests for pattern selection #863

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Add tests for pattern selection #863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c24339
Add tests for pattern selection
eemeli Aug 21, 2024
034c512
Add missing errors
eemeli Aug 21, 2024
cc164ee
Apply suggestions from code review
eemeli Aug 21, 2024
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
33 changes: 29 additions & 4 deletions test/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,23 +65,34 @@ The function `:test:function` requires a [Number Operand](/spec/registry.md#numb

#### Options

The only _option_ `:test:function` recognizes is `decimalPlaces`,
a _digit size option_ for which only `0` and `1` are valid values.
The following _options_ are available on `:test:function`:
- `decimalPlaces`, a _digit size option_ for which only `0` and `1` are valid values.
- `0`
- `1`
- `fails`
- `never` (default)
- `select`
- `format`
- `always`
Comment on lines +73 to +76
Copy link
Member

Choose a reason for hiding this comment

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

are these sufficiently self-documenting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're somewhat documented by the behaviour section below. Not explicitly documenting these is also in line with what we've done for all the other functions' option values.


All other _options_ and their values are ignored.

#### Behavior

When resolving a `:test:function` expression,
its `Input` and `DecimalPlaces` values are determined as follows:
its `Input`, `DecimalPlaces`, `FailsFormat`, and `FailsSelect` values are determined as follows:

1. Let `DecimalPlaces` be 0.
1. Let `FailsFormat` be `false`.
1. Let `FailsSelect` be `false`.
1. Let `arg` be the resolved value of the _expression_ _operand_.
1. If `arg` is the resolved value of an _expression_
with a `:test:function`, `:test:select`, or `:test:format` _annotation_
for which resolution has succeeded, then
1. Let `Input` be the `Input` value of `arg`.
1. Set `DecimalPlaces` to be `DecimalPlaces` value of `arg`.
1. Set `FailsFormat` to be `FailsFormat` value of `arg`.
1. Set `FailsSelect` to be `FailsSelect` value of `arg`.
1. Else if `arg` is a numerical value
or a string matching the `number-literal` production, then
1. Let `Input` be the numerical value of `arg`.
Expand All @@ -96,12 +107,24 @@ its `Input` and `DecimalPlaces` values are determined as follows:
1. Else if its value is not an unresolved value set by _option resolution_,
1. Emit "bad-option" _Resolution Error_.
1. Use a _fallback value_ as the resolved value of the _expression_.
1. If the `fails` _option_ is set, then
1. If its value resolves to the string `'always'`, then
1. Set `FailsFormat` to be `true`.
1. Set `FailsSelect` to be `true`.
1. Else if its value resolves to the string `'format'`, then
1. Set `FailsFormat` to be `true`.
1. Else if its value resolves to the string `'select'`, then
1. Set `FailsSelect` to be `true`.
1. Else if its value does not resolve to the string `'never'`, then
1. Emit "bad-option" _Resolution Error_.

When `:test:function` is used as a _selector_,
the behaviour of calling it as the `rv` value of MatchSelectorKeys(`rv`, `keys`)
(see [Resolve Preferences](/spec/formatting.md#resolve-preferences) for more information)
depends on its `Input` and `DecimalPlaces` values.
depends on its `Input`, `DecimalPlaces` and `FailsSelect` values.

- If `FailsSelect` is `true`,
calling the method will fail 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'` »,
depending on whether those values are included in `keys`.
Expand All @@ -128,6 +151,8 @@ If the formatting target is a sequence of parts,
each of the above parts will be emitted separately
rather than being concatenated into a single string.

If `FailsFormat` is `true`,
attempting to format the _placeholder_ to any formatting target will fail.

### `:test:select`

Expand Down
167 changes: 0 additions & 167 deletions test/tests/functions/number.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,173 +209,6 @@
}
]
},
{
"src": ".match {$foo :number} one {{one}} * {{other}}",
Copy link
Member

Choose a reason for hiding this comment

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

This file tests the function :number (and its friend :integer, which are specified as part of the standard function set*. Won't we need these tests for that purpose? The file is named appropriately for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tests removed from here rely on the category selection behaviour of :number, which is not strictly speaking required of an implementation, as we only say that "selection should be based on CLDR plural rule data". If that wiggle room is not intentional, then we should (separately from this PR) firm that up, and possibly introduce more specific tests for :number that check a wider range of locales.

These tests also mostly exercise the MF2 spec side of pattern selection, for which each case is now included in the :test:select tests.

Copy link
Member

Choose a reason for hiding this comment

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

We probably should fix these tests to use CLDR data, if that's what we want. My primary concern is not to lose tests for something that is part of our collection of specifications, even if it isn't part of MF2's core spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we keep these :number tests in as they are, we're requiring that a "should" in the spec is treated the same as a "MUST". If we do want to test this behaviour, the tests for it ought to be separate and somehow marked so that an implementation that chooses to do something else can ignore them.

The spec-required parts of these tests is completely retained in the :test:select tests added in this PR.

I added #868 to continue the conversation about the spec language for :number and :integer selection.

"params": [
{
"name": "foo",
"value": 1
}
],
"exp": "one"
},
{
"src": ".match {$foo :number} 1 {{=1}} one {{one}} * {{other}}",
"params": [
{
"name": "foo",
"value": 1
}
],
"exp": "=1"
},
{
"src": ".match {$foo :number} one {{one}} 1 {{=1}} * {{other}}",
"params": [
{
"name": "foo",
"value": 1
}
],
"exp": "=1"
},
{
"src": ".match {$foo :number} {$bar :number} one one {{one one}} one * {{one other}} * * {{other}}",
"params": [
{
"name": "foo",
"value": 1
},
{
"name": "bar",
"value": 1
}
],
"exp": "one one"
},
{
"src": ".match {$foo :number} {$bar :number} one one {{one one}} one * {{one other}} * * {{other}}",
"params": [
{
"name": "foo",
"value": 1
},
{
"name": "bar",
"value": 2
}
],
"exp": "one other"
},
{
"src": ".match {$foo :number} {$bar :number} one one {{one one}} one * {{one other}} * * {{other}}",
"params": [
{
"name": "foo",
"value": 2
},
{
"name": "bar",
"value": 2
}
],
"exp": "other"
},
{
"src": ".input {$foo :number} .match {$foo} one {{one}} * {{other}}",
"params": [
{
"name": "foo",
"value": 1
}
],
"exp": "one"
},
{
"src": ".local $foo = {$bar :number} .match {$foo} one {{one}} * {{other}}",
"params": [
{
"name": "bar",
"value": 1
}
],
"exp": "one"
},
{
"src": ".input {$foo :number} .local $bar = {$foo} .match {$bar} one {{one}} * {{other}}",
"params": [
{
"name": "foo",
"value": 1
}
],
"exp": "one"
},
{
"src": ".input {$bar :number} .match {$bar} one {{one}} * {{other}}",
"params": [
{
"name": "bar",
"value": 2
}
],
"exp": "other"
},
{
"src": ".input {$bar} .match {$bar :number} one {{one}} * {{other}}",
"params": [
{
"name": "bar",
"value": 1
}
],
"exp": "one"
},
{
"src": ".input {$bar} .match {$bar :number} one {{one}} * {{other}}",
"params": [
{
"name": "bar",
"value": 2
}
],
"exp": "other"
},
{
"src": ".input {$none} .match {$foo :number} one {{one}} * {{{$none}}}",
"params": [
{
"name": "foo",
"value": 1
}
],
"exp": "one"
},
{
"src": ".local $bar = {$none} .match {$foo :number} one {{one}} * {{{$bar}}}",
"params": [
{
"name": "foo",
"value": 1
}
],
"exp": "one"
},
{
"src": ".local $bar = {$none} .match {$foo :number} one {{one}} * {{{$bar}}}",
"params": [
{
"name": "foo",
"value": 2
}
],
"exp": "{$none}",
"expErrors": [
{
"type": "unresolved-variable"
}
]
},
{
"src": "{42 :number @foo @bar=13}",
"exp": "42",
Expand Down
150 changes: 150 additions & 0 deletions test/tests/pattern-selection.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
{
"$schema": "https://raw.githubusercontent.com/unicode-org/message-format-wg/main/test/schemas/v0/tests.schema.json",
"scenario": "Pattern selection",
"description": "Tests for pattern selection",
"defaultTestProperties": {
"locale": "und"
},
"tests": [
{
"src": ".match {1 :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"exp": "1"
},
{
"src": ".match {0 :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"exp": "other"
},
{
"src": ".match {$x :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1"
},
{
"src": ".match {$x :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 2 }],
"exp": "other"
},
{
"src": ".input {$x} .match {$x :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1"
},
{
"src": ".input {$x} .match {$x :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 2 }],
"exp": "other"
},
{
"src": ".input {$x :test:select} .match {$x} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1"
},
{
"src": ".input {$x :test:select} .match {$x} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 2 }],
"exp": "other"
},
{
"src": ".input {$x :test:select} .local $y = {$x} .match {$y} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1"
},
{
"src": ".input {$x :test:select} .local $y = {$x} .match {$y} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 2 }],
"exp": "other"
},
{
"src": ".match {1 :test:select decimalPlaces=1} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"exp": "1.0"
},
{
"src": ".match {1 :test:select decimalPlaces=1} 1 {{1}} 1.0 {{1.0}} * {{other}}",
"exp": "1.0"
},
{
"src": ".match {1 :test:select decimalPlaces=9} 1.0 {{1.0}} 1 {{1}} * {{bad-option-value}}",
"exp": "bad-option-value",
"expErrors": [{ "type": "bad-option" }, { "type": "bad-selector" }]
},
{
"src": ".input {$x :test:select} .match {$x :test:select decimalPlaces=1} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1.0"
},
{
"src": ".input {$x :test:select decimalPlaces=1} .match {$x :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1.0"
},
{
"src": ".input {$x :test:select} .local $y = {$x :test:select decimalPlaces=1} .match {$y} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1.0"
},
{
"src": ".input {$x :test:select decimalPlaces=1} .local $y = {$x :test:select} .match {$y} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "1.0"
},
{
"src": ".input {$x :test:select decimalPlaces=9} .match {$x :test:select decimalPlaces=1} 1.0 {{1.0}} 1 {{1}} * {{bad-option-value}}",
"params": [{ "name": "x", "value": 1 }],
"exp": "bad-option-value",
"expErrors": [
{ "type": "bad-option" },
{ "type": "bad-operand" },
{ "type": "bad-selector" }
]
},
{
"src": ".match {1 :test:select fails=select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"exp": "other",
"expErrors": [{ "type": "bad-selector" }]
},
{
"src": ".match {1 :test:select fails=format} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"exp": "1"
},
{
"src": ".match {1 :test:format} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"exp": "other",
"expErrors": [{ "type": "bad-selector" }]
},
{
"src": ".match {$x :test:select} 1.0 {{1.0}} 1 {{1}} * {{other}}",
"exp": "other",
"expErrors": [
{ "type": "unresolved-variable" },
{ "type": "bad-operand" },
{ "type": "bad-selector" }
]
},
{
"src": ".match {1 :test:select} {1 :test:select} 1 1 {{1,1}} 1 * {{1,*}} * 1 {{*,1}} * * {{*,*}}",
"exp": "1,1"
},
{
"src": ".match {1 :test:select} {0 :test:select} 1 1 {{1,1}} 1 * {{1,*}} * 1 {{*,1}} * * {{*,*}}",
"exp": "1,*"
},
{
"src": ".match {0 :test:select} {1 :test:select} 1 1 {{1,1}} 1 * {{1,*}} * 1 {{*,1}} * * {{*,*}}",
"exp": "*,1"
},
{
"src": ".match {0 :test:select} {0 :test:select} 1 1 {{1,1}} 1 * {{1,*}} * 1 {{*,1}} * * {{*,*}}",
"exp": "*,*"
},
{
"src": ".match {1 :test:select fails=select} {1 :test:select} 1 1 {{1,1}} 1 * {{1,*}} * 1 {{*,1}} * * {{*,*}}",
"exp": "*,1",
"expErrors": [{ "type": "bad-selector" }]
},
{
"src": ".match {1 :test:select} {1 :test:format} 1 1 {{1,1}} 1 * {{1,*}} * 1 {{*,1}} * * {{*,*}}",
"exp": "1,*",
"expErrors": [{ "type": "bad-selector" }]
}
]
}