CLDR-17644 Add additional test data for locale display name algorithm #3728
+2,917
−21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
macchiati
reviewed
May 16, 2024
| // only when we match do we get & process the data at that path | ||
| if (path.startsWith(LANGUAGE_PATH)) { | ||
| // Get display name | ||
| String value = cldrFile.getStringValue(path); |
There was a problem hiding this comment.
Each cldrFile has a method to create a locale name (optionally with language, script, and region). It would be better to use that.
Clearly we don't want to produce all the combinations.
I would suggest. Take the locales in Organization.cldr (call that LOC)
for locale in LOC
for localeForName in LOC
test to see if each localeForName's pieces is in Organization.cldr's coverage for locale
skip if not
emit the locale identifier and the name of localeForName in locale
An enhancement of that would be to also have alt values.
There was a problem hiding this comment.
After more work with @sffc , we pared down the generated test data to a small list of input locales that exercise interesting corner cases (lower & higher coverage levels, which pieces of LSR are used to form the dialect display name, etc.).
We also moved the logic into a preexisting test data file generator (rather than creating a new class file of code and output file with a duplicate purpose).
We went ahead and ensure that both the {standard, dialect} styles of display are used to generate test cases, whereas it previously only used standard.
…Data; add extra input locales; reduce formatting locales to those that have no 'junk' data
…isting test file generator
|
One question we had: it seems that reading data from |
macchiati
reviewed
May 23, 2024
| @@ -3919,6 +3919,10 @@ public synchronized String getName( | |||
| if (localePattern == null) { | |||
| localePattern = "{0} ({1})"; | |||
| } | |||
| // Hack | |||
| if (localeSeparator == null) { | |||
There was a problem hiding this comment.
Hmmm. This should never happen. Can you see which locales this occurs in?
There was a problem hiding this comment.
It occurs in about three-quarters of locales, including major ones like it, de. See my comment about my suspicion that they aren't following fallback
There was a problem hiding this comment.
This is fixed per our discussion earlier!
We also switched to using CalculatedCoverageLevels, which cleans up a lot of code and output.
Does the PR look good now?
|
There is a test failure: |
sffc
reviewed
Jun 4, 2024
Comment on lines
197
to
200
| CLDRFile cldrFile = | ||
| factory.make(locale, true, DraftStatus.contributed); // don't include | ||
| // draft=unconfirmed/provisional | ||
| CLDRFile unresolved = cldrFile.getUnresolved(); |
There was a problem hiding this comment.
@macchiati: if you pass true in factory.make, it should fall back to root. The unresolved one does not fall back to root. It looks like it's getting display names with the unresolved version. If you have the unresolved version, it does not fall back to root, but it falls back to code. For example, if there's no value, you return the code. For example, display name of "en-GB" becomes "en (GB)". You probably want to test that because it is in the spec. The resolved version still uses code fallback.
Alternatively, there is a method on CLDRFile that tells you where the string comes from. What it returns is that, for a particular value, what locale is it coming from? It also tells you if the path changed when fetching. That's probably not relevant to this code, but that detail is available.
sffc
reviewed
Jun 4, 2024
| @@ -50,6 +50,7 @@ adp ; dz | |||
| afr ; af | |||
There was a problem hiding this comment.
Note: There's a tool GenerateAllTests (?) that runs all the tools.
sffc
reviewed
Jun 4, 2024
| @@ -50,6 +50,7 @@ adp ; dz | |||
| afr ; af | |||
| agp ; apf | |||
There was a problem hiding this comment.
Note: There's a class called ICUServiceBuilder that plugs CLDR data into ICU formatters, including number and date formatting.
sffc
changed the title
echeran
changed the title
|
Hi @macchiati, we almost resolved all the test failures, but we need your help to resolve the final 4 remaining assertion failures: The test data hasn't changed, so we're not sure where the translation data for the IPA variant is coming from. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CLDR-17644