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 tests for pattern selection #863

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Add tests for pattern selection #863

merged 3 commits into from
Aug 26, 2024

Conversation

eemeli
Copy link
Collaborator

@eemeli eemeli commented Aug 21, 2024

Currently, the tests we have for pattern selection rely on the behaviour of :number, and are somewhat incomplete. We should instead use :test:select and :test:format for this, and work to ensure more aspects are covered.

A new fails option is added to the the test functions, to allow for the changes made in #828 to be testable (i.e. what happens if a custom selector function throws an error during MatchSelectorKeys).

The .match tests on :number are all moved to the new test suite.

Comment on lines +73 to +76
- `never` (default)
- `select`
- `format`
- `always`
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.

@@ -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.

test/tests/pattern-selection.json Outdated Show resolved Hide resolved
test/tests/pattern-selection.json Outdated Show resolved Hide resolved
test/tests/pattern-selection.json Outdated Show resolved Hide resolved
Co-authored-by: Addison Phillips <[email protected]>
@eemeli eemeli requested a review from aphillips August 21, 2024 16:05
@aphillips aphillips merged commit b4fbf04 into main Aug 26, 2024
2 checks passed
@aphillips aphillips deleted the test-pattern-selection branch August 26, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants