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

CLDR-17563 fix unclear test message about attributeValueValidity.xml #3647

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CLDR-17563 fix unclear test message about attributeValueValidity.xml #3647

wants to merge 1 commit into from
+6 −16

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Apr 19, 2024 •
edited

  • fix an internal function for clarity
  • change a constant to a property
  • be less cryptic
  • drop debug code

CLDR-17563

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

Example run (fake of course):

-
[INFO] Running org.unicode.cldr.unittest.TestShim
org.unicode.cldr.unittest.testArgs=-n -q -filter:testLSR
CLDR {
  TestCoverageLevel {
    testLSR {
      Error: (TestCoverageLevel.java:1280) : $language in attributeValueValidity.xml needs to include these languages: (7): expected "", got "hnj ie ken kxv lld osa quc"
    } (0.455s) FAILED (1 failure(s), 1 warning(s))
  } (0.462s) FAILED (1 failure(s), 1 warning(s), 11 test(s) skipped)
} (8.012s) FAILED (1 failure(s), 5 warning(s), 496 test(s) skipped)

Error summary:

@srl295 srl295 self-assigned this Apr 19, 2024
@srl295
CLDR-17563 fix unclear test message about attributeValueValidity.xml
7ba8ec9 
- fix an internal function for clarity
- change a constant to a property
- [X] be less cryptic
- drop debug code
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295 srl295 changed the title CLDR-16818 fix unclear test message about attributeValueValidity.xml CLDR-17563 fix unclear test message about attributeValueValidity.xml Apr 19, 2024
@srl295
Copy link
Member Author

srl295 commented Apr 19, 2024

trying again without using an unrelated ticket ID

@DavidLRowe
Copy link
Contributor

Not clear on the different ticket numbers (CLDR-17563 in title, CLDR-16818 in description).

From the little I know about Java, the error message seems much more informative!

@srl295
Copy link
Member Author

srl295 commented Apr 19, 2024 •
edited by jira bot

Not clear on the different ticket numbers (CLDR-17563 in title, CLDR-16818 in description).

From the little I know about Java, the error message seems much more informative!

Fixed

macchiati
macchiati requested changes Apr 19, 2024
View reviewed changes
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestCoverageLevel.java
@@ -1280,7 +1278,7 @@ public void testLSR() {
}
}
if (!assertEquals(
"Language subtags that are in a CLDR locale's ID are in root ("
"$language in attributeValueValidity.xml needs to include these languages: ("
Copy link
Member

Choose a reason for hiding this comment

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

This is misleading in a different way. The error just indicates that there is a mismatch.

Suggested change
"$language in attributeValueValidity.xml needs to include these languages: ("
"There is a mismatch: each language in the following list either needs to be removed from root.xml, OR added to $language in attributeValueValidity.xml: ("

Copy link
Member Author

Choose a reason for hiding this comment

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

What does "removed from root.xml" mean? In the reproducer, "lld" wasn't in root.xml.

Copy link
Member Author

Choose a reason for hiding this comment

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

@macchiati any clarification on this?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm. Why was lld failing; was it for not being in validity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. lld had not been added to validity element in attributeValueValidity.xml

Copy link
Contributor

Choose a reason for hiding this comment

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

#3666 changes this wording to: "Language subtags in a locale's ID must be in one of the attributeValueValidity.xml $language* sets, typically $languageNonTcLtBasic (" which is even clearer. I'd also recommend dropping the message change from this PR.

@macchiati
Copy link
Member

macchiati commented Apr 19, 2024 via email

@srl295
Copy link
Member Author

srl295 commented Apr 19, 2024

I won't have time to look at this today.

The other data pr is unblocked. This is just to clarify.

@macchiati
Copy link
Member

As a part of #3666, I fixed the messages and added a lot of comments about what is going on in that section. Should be much easier for people to see what is going on. If that is approved, then I think you could drop the rewording part of this PR (but keep the fix to SHOW_LSR_DATA)

DavidLRowe
DavidLRowe reviewed May 2, 2024
View reviewed changes
Copy link
Contributor

@DavidLRowe DavidLRowe left a comment

Choose a reason for hiding this comment

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

see recommendation to drop message change (since another PR will address that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidLRowe DavidLRowe DavidLRowe left review comments

@macchiati macchiati macchiati requested changes

@btangmu btangmu Awaiting requested review from btangmu

Requested changes must be addressed to merge this pull request.

Assignees

@srl295 srl295

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@srl295 @DavidLRowe @macchiati