Skip to content

Conversation

@catamorphism
Copy link
Collaborator

In trying to integrate the spec tests directly into the ICU4C tests (without modifying the JSON files), I noticed an inconsistency between the time and datetime functions' output as specified in the test, and what ICU outputs. (I'd noticed this before, but as I was importing the tests manually, I changed the expected output manually.)

ICU puts a narrow non-breaking space (0x202F) before the "AM" or "PM" in formatted date/time strings, which is a requirement from CLDR. I did a quick test that JS (at least running in Node) does the same thing, and it does:

$ node
Welcome to Node.js v18.13.0.
Type ".help" for more information.
> s = (Intl.DateTimeFormat("en-us", { timeStyle: "long" }).format(Date.now()))
'5:28:32 PM PDT'
> s.charCodeAt(7).toString(16)
'202f'

This change makes the expected output for time and datetime match what ICU outputs in these cases.

ICU uses a narrow non-breaking space (0x202F) in formatted date/time
strings, which is based on CLDR.

This change makes the expected output for time and datetime match
what ICU outputs in these cases.
@catamorphism catamorphism requested review from aphillips and eemeli April 4, 2024 00:30
@catamorphism
Copy link
Collaborator Author

A discussion could be had about what these tests should test for, since the spec doesn't specify exactly how dates/times are formatted. But if the tests are going to be used for checking implementations' conformance, and the expected output of test cases is a single hard-coded string, I think this is the right change. Maybe in the longer term the expected output should be specified differently. The exp strings could be replaced with regexes (at least to treat all whitespace characters the same way).

What I want to use the tests for is to make sure my implementation is handling all the options to :date/:time/:datetime correctly, so it's useful to me if the output matches what CLDR requires, so that I don't have to use a modified version of the spec .json files or something nasty like that.

@aphillips
Copy link
Member

I think all of these tests need to be refactored--and not to contain any date/time formats at all. The tests for built-in functions should test that there is a required function, that it accepts the required literals (implementation defined types are up to the implementation), that it accepts the required options with the values specified by the spec, and that it emits the errors required.

The exception to this might be the string formatter.

If we want to test actual formatting, that belongs in ICU or maybe CLDR.

Copy link
Collaborator

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

This specific change is in fact a rather good example of why we should not try to match the specific output of ICU. The change in the space character separating the AM/PM marker was made in CLDR 42, and prompted the following browser patches:

If you test your code with Node.js 20 or later, you should find that it'll use a plain space rather than \u202f.

@catamorphism
Copy link
Collaborator Author

So should we just remove the expected output for the tests in test-functions.json (except for :string), so as to specify that these strings should be formatted without errors, but the spec tests don't require a particular output?

@aphillips
Copy link
Member

(as contributor)

Yes, that's my opinion. Eventually a bit more than that. But not create tests that depend on the implementation or on specific CLDR data in this particular test suite.

@catamorphism
Copy link
Collaborator Author

Closing in favor of #760

@eemeli eemeli added this to the LDML 46 milestone Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

out-of-scope? test-suite Issue pertains to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants