ICU-22794 MF2: De-duplicate C++ and Java test data #3050

catamorphism · 2024-06-26T22:18:00Z

This PR moves all the .json files for data-driven ICU4C and ICU4J tests into a single top-level directory, icu/testdata/.

I did my best to remove duplicate tests.

This involved fixing several ICU4J parser bugs, almost all involving whitespace handling.

Some tests still had to be ignored in either Java or C++, in which cases I filed ICU tickets.

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22794
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

jira-pull-request-webhook · 2024-06-27T21:22:21Z

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

~ Your Friendly Jira-GitHub PR Checker Bot

catamorphism · 2024-06-27T21:25:24Z

/cc @echeran @mihnita

srl295 · 2024-06-28T21:26:06Z

Maybe this was discussed before, but for example ICU tarballs in https://github.com/unicode-org/icu/releases/tag/release-75-1 do not have this format.

Perhaps we should have a situation where the testdata gets copied in the ICU4C and ICU4J tarballs at a high level? I.e. so source and testdata are siblings.

Then the code could check ../testdata (source tarball) and if that doesn't exist then check ../../testdata (in-repo) and then fail.

srl295 · 2024-06-28T21:40:35Z

icu4c/source/test/intltest/intltest.cpp

+#else
+    srcDataDir = ".." U_FILE_SEP_STRING ".." U_FILE_SEP_STRING ".." U_FILE_SEP_STRING ".." U_FILE_SEP_STRING "testdata" U_FILE_SEP_STRING;
+    FILE *f = fopen(".." U_FILE_SEP_STRING ".." U_FILE_SEP_STRING "test" U_FILE_SEP_STRING
+                      "testdata" U_FILE_SEP_STRING "rbbitst.txt",


did you move rbbitst.txt in this PR? I think this should perhaps be a different file?

I think it's correct as-is, but it's a weird way to check if a directory exists (I copied that from existing code). In 0ef54a5 I changed it to call stat() on the testdata directory itself instead.

icu4c/source/test/intltest/intltest.h

srl295 · 2024-06-28T22:00:59Z

icu4j/main/core/src/main/java/com/ibm/icu/message2/MFParser.java

-                checkCondition(cp == '|', "Invalid escape sequence, only \"\\|\" is valid here");
-                result.appendCodePoint('|');
+                boolean isValidEscape = cp == '|' || cp == '\\' || cp == '{' || cp == '}';
+                checkCondition(isValidEscape, "Invalid escape sequence inside quoted literal");


This seems to validate the approach of shared test data!

srl295 · 2024-06-28T22:32:23Z

icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/TestUtils.java

+        Path icuTestdataDir = filePath.resolve("../../../../../../../../../../../testdata/message2/").normalize();
+        return icuTestdataDir.resolve(json);


Ideally should test and give the user some feedback if it doesn't work. Also, see comment about packaging when using a non-repo work area.

Done in c53f203. Re: using a non-repo work area, see #3050 (comment)

testdata/message2/README.txt

srl295 · 2024-06-28T22:37:02Z

testdata/message2/README.txt

+Tests in the `spec/` subdirectory are taken from https://github.com/unicode-org/message-format-wg/blob/main/test
+and need to be manually updated if the contents change upstream.


Suggested change

Tests in the `spec/` subdirectory are taken from https://github.com/unicode-org/message-format-wg/blob/main/test

and need to be manually updated if the contents change upstream.

Tests in the `spec/` subdirectory are taken from https://github.com/unicode-org/message-format-wg/blob/main/test

and need to be manually updated if the contents change upstream, but see https://unicode-org.atlassian.net/browse/ICU-22812 for making that automatic.

wouldn't it be from CLDR not mfwg?

Sort of -- if MFWG changes the spec tests, then CLDR would need to change to reflect those changes (for non-spec tests, CLDR is the source of truth). Clarified this in 1993b61.

catamorphism · 2024-07-17T22:46:26Z

Maybe this was discussed before, but for example ICU tarballs in https://github.com/unicode-org/icu/releases/tag/release-75-1 do not have this format.

Perhaps we should have a situation where the testdata gets copied in the ICU4C and ICU4J tarballs at a high level? I.e. so source and testdata are siblings.

Then the code could check ../testdata (source tarball) and if that doesn't exist then check ../../testdata (in-repo) and then fail.

In ICU4C, 0ef54a5 should do this.

I'm not sure how to do the same for ICU4J, though. I got as far as finding the icu4j/releases_tools/github_release.sh, but I can't get it to run without errors, even in a branch with no local changes. I pasted the relevant part of the output into https://gist.github.com/catamorphism/1c9ea0e26580dd18ef69852de931ce8d . It's giving me an error about the javadocs. So I can't get far enough to make the change to copy the testdata/ directory into the jar file and test the change. Do you have any suggestions?

catamorphism · 2024-07-17T23:57:23Z

I'm not sure how to do the same for ICU4J, though. I got as far as finding the icu4j/releases_tools/github_release.sh, but I can't get it to run without errors, even in a branch with no local changes. I pasted the relevant part of the output into https://gist.github.com/catamorphism/1c9ea0e26580dd18ef69852de931ce8d . It's giving me an error about the javadocs. So I can't get far enough to make the change to copy the testdata/ directory into the jar file and test the change. Do you have any suggestions?

I was able to get it to work by editing the github_release.sh script as follows:

# mvn clean install -DskipITs -DskipTests -P with_sources,with_javadoc
mvn clean install -DskipITs -DskipTests -P with_sources
mvn install -DskipITs -DskipTests -P with_javadoc

though I don't know if that breaks anything. However, I'm not sure how to run the tests in the source .jar file once it's been created.

catamorphism · 2024-07-25T19:10:53Z

Looks like the build failures are timeouts.

srl295 · 2024-07-31T21:49:49Z

I'm not sure how to do the same for ICU4J, though. I got as far as finding the icu4j/releases_tools/github_release.sh, but I can't get it to run without errors, even in a branch with no local changes. I pasted the relevant part of the output into https://gist.github.com/catamorphism/1c9ea0e26580dd18ef69852de931ce8d . It's giving me an error about the javadocs.

that has to do with needing to use an older java to build.

So I can't get far enough to make the change to copy the testdata/ directory into the jar file and test the change. Do you have any suggestions?

I'm not sure, but I can also try to test/fix this in the PR #3053

I was able to get it to work by editing the github_release.sh script as follows:
# mvn clean install -DskipITs -DskipTests -P with_sources,with_javadoc
mvn clean install -DskipITs -DskipTests -P with_sources
mvn install -DskipITs -DskipTests -P with_javadoc
though I don't know if that breaks anything. However, I'm not sure how to run the tests in the source .jar file once it's been created.

You have to unpack the jar and then compile everything.

icu4c/source/test/intltest/intltest.cpp

srl295

seems to be in the right direction, need to verify with ICU-TC whether sys/stat.h is OK for inclusion.

srl295 · 2024-07-31T23:05:36Z

@catamorphism OK, well all tests pass on ICU4C (verified they are loading the files from the common area) and on ICU4J they fail with this (which at least shows that the test data is being read from its location!)

[ERROR] Failures: 
[ERROR]   FromJsonTest.test:417 TestCase {
  message: {hello {(world)}}',
  locale: 'en-US',
  arguments: {},
  expected: 'hello world',
  ignore: false,
  ignoreReason: '',
  errors: []
}
No error was expected here, but it happened:
Parse error:
Message: <<{hello {(world)}}>>
Error: Parse error [8]: Unclosed placeholder
{hello {^^^(world)}}

[ERROR]   FunctionsTest.test:32 UnitTest {src="Default int64: {$val}!", params={val=1.2345678901234568E18}, exp="Default int64: 1.234.567.890.123.456.770!"} expected:<...234.567.890.123.456.[77]0!> but was:<...234.567.890.123.456.[80]0!>
[INFO] 
[ERROR] Tests run: 2833, Failures: 2, Errors: 0, Skipped: 26

catamorphism · 2024-08-01T01:10:38Z

though I don't know if that breaks anything. However, I'm not sure how to run the tests in the source .jar file once it's been created.

You have to unpack the jar and then compile everything.

Do any of the .jar files actually contain tests? I noticed that the github_release.sh script passes the flag -DskipTests when creating files, and after running the script, there are no test files in icu4j/target/github_release/icu4j-76_0_1-sources.jar.

catamorphism · 2024-08-01T01:42:13Z

seems to be in the right direction, need to verify with ICU-TC whether sys/stat.h is OK for inclusion.

No need, I changed it to check for a text file.

catamorphism · 2024-08-01T02:11:15Z

@catamorphism OK, well all tests pass on ICU4C (verified they are loading the files from the common area) and on ICU4J they fail with this (which at least shows that the test data is being read from its location!)

What command are you running to get that output? It's weird because the string {hello {(world)}} doesn't appear anywhere in either top-level icu4j or testdata in this branch, nor does it appear in icu4j in the main branch.

echeran

LGTM. Thanks for refactoring test data & getting ICU4J tests to use the entire shared test data set including fixing up ICU4J unit test code accordingly.

…rectory Modify ICU4C and ICU4J test readers to handle all tests Add `ignoreJava` and `ignoreCpp` properties to tests where needed Includes parser bug fixes: ICU4J: require a complex-body after declarations ICU4J: Correctly parse the complex body after an unsupported statement ICU4J: Handle date params in tests and remove default params for tests ICU4J: Handle decimal params in tests ICU4J: Require whitespace before variable/literal in reserved annotation ICU4J: Require whitespace between options ICU4J: Require a variable-expression in an .input declaration ICU4J: don't require space between last key and pattern in variant ICU4J: don't require space between selectors ICU4J: allow whitespace after '=' in option ICU4J: parse escape sequences in quoted literals according to grammar ICU4J: allow whitespace within markup after attributes list

jira-pull-request-webhook · 2024-08-07T14:18:11Z

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

~ Your Friendly Jira-GitHub PR Checker Bot

srl295

I think this is ready to go

markusicu

thank you for the good work!
some questions...

markusicu · 2024-08-07T23:27:40Z

icu4c/source/config/dist.mk

+    # Copy top-level testdata directory so it's a sibling of the source/ directory
+	cp -R $(ICU4CTOP)/../testdata $(DISTY_TMP)/icu


Why are we copying testdata files?
Is it because we distribute ICU4C .zip/.jar files rooted in icu4c/ rather than the one-higher repo root?
If so, shouldn't we change that instead? E.g., start with 76 to package up from the repo root, but for ICU4C exclude the icu4j/ and tools/ folders (and probably some more stuff)?

markusicu · 2024-08-07T23:28:18Z

icu4c/source/test/intltest/intltest.cpp

@@ -1679,6 +1679,61 @@ const char *IntlTest::getSourceTestData(UErrorCode& /*err*/) {
    return srcDataDir;
 }

+static bool fileExists(const char* fileName) {
+    // Test for `srcDataDir` existing by checking for `srcDataDir`/message2/valid-tests.json


This seems strangely, overly specific for this rather generic function.
Did this want to be a comment on or in getSharedTestData()?

markusicu · 2024-08-07T23:28:55Z

icu4c/source/test/intltest/intltest.cpp

+static bool fileExists(const char* fileName) {
+    // Test for `srcDataDir` existing by checking for `srcDataDir`/message2/valid-tests.json
+    U_ASSERT(fileName != nullptr);
+    FILE *f = fopen(fileName, "r");


Can/should we use some modern-ish C++ library function for "file exists"?

markusicu · 2024-08-07T23:40:45Z

icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/TestUtils.java

+        // First, check the top level of the source directory,
+        // in case we're in a source tarball


Same question as for ICU4C: Should we start to package up ICU4J zip/jar/tar files from repo root, except omit unnecessary folders?

catamorphism force-pushed the icu4c-and-j-test-unification branch from 0071651 to 3503550 Compare June 27, 2024 21:22

catamorphism marked this pull request as ready for review June 27, 2024 21:25

srl295 requested review from srl295, mihnita and echeran June 28, 2024 20:38

srl295 changed the title ~~[ICU-22794] MF2: De-duplicate C++ and Java test data~~ ICU-22794 MF2: De-duplicate C++ and Java test data Jun 28, 2024

srl295 reviewed Jun 28, 2024

View reviewed changes

markusicu assigned echeran Jul 18, 2024

markusicu self-requested a review July 18, 2024 16:03

catamorphism mentioned this pull request Jul 22, 2024

ICU-22834 MF2: make tests compliant with schema and update spec tests #3063

Merged

7 tasks

srl295 reviewed Jul 31, 2024

View reviewed changes

icu4c/source/test/intltest/intltest.cpp Outdated Show resolved Hide resolved

srl295 previously approved these changes Jul 31, 2024

View reviewed changes

catamorphism dismissed srl295’s stale review via b8dfad4 August 1, 2024 01:41

echeran approved these changes Aug 6, 2024

View reviewed changes

catamorphism force-pushed the icu4c-and-j-test-unification branch from a7e238e to dd6d64c Compare August 7, 2024 14:18

srl295 approved these changes Aug 7, 2024

View reviewed changes

markusicu reviewed Aug 7, 2024

View reviewed changes

echeran merged commit 57ed0a2 into unicode-org:main Aug 8, 2024
105 checks passed

catamorphism mentioned this pull request Aug 8, 2024

ICU-22763 MF2: Handle time zones correctly in :datetime #3012

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-22794 MF2: De-duplicate C++ and Java test data #3050

ICU-22794 MF2: De-duplicate C++ and Java test data #3050

catamorphism commented Jun 26, 2024 •

edited

Loading

jira-pull-request-webhook bot commented Jun 27, 2024

catamorphism commented Jun 27, 2024

srl295 commented Jun 28, 2024

srl295 Jun 28, 2024

catamorphism Jul 17, 2024

srl295 Jun 28, 2024

srl295 Jun 28, 2024

catamorphism Jul 17, 2024

srl295 Jun 28, 2024 •

edited

Loading

catamorphism Jul 17, 2024

catamorphism commented Jul 17, 2024

catamorphism commented Jul 17, 2024 •

edited

Loading

catamorphism commented Jul 25, 2024

srl295 commented Jul 31, 2024

srl295 left a comment

srl295 commented Jul 31, 2024 •

edited

Loading

catamorphism commented Aug 1, 2024

catamorphism commented Aug 1, 2024 •

edited

Loading

catamorphism commented Aug 1, 2024 •

edited

Loading

echeran left a comment

jira-pull-request-webhook bot commented Aug 7, 2024

srl295 left a comment

markusicu left a comment

markusicu Aug 7, 2024

markusicu Aug 7, 2024

markusicu Aug 7, 2024

markusicu Aug 7, 2024

		Path icuTestdataDir = filePath.resolve("../../../../../../../../../../../testdata/message2/").normalize();
		return icuTestdataDir.resolve(json);

		Tests in the `spec/` subdirectory are taken from https://github.com/unicode-org/message-format-wg/blob/main/test
		and need to be manually updated if the contents change upstream.

		# Copy top-level testdata directory so it's a sibling of the source/ directory
		cp -R $(ICU4CTOP)/../testdata $(DISTY_TMP)/icu

		// First, check the top level of the source directory,
		// in case we're in a source tarball

ICU-22794 MF2: De-duplicate C++ and Java test data #3050

ICU-22794 MF2: De-duplicate C++ and Java test data #3050

Conversation

catamorphism commented Jun 26, 2024 • edited Loading

Checklist

jira-pull-request-webhook bot commented Jun 27, 2024

catamorphism commented Jun 27, 2024

srl295 commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catamorphism commented Jul 17, 2024

catamorphism commented Jul 17, 2024 • edited Loading

catamorphism commented Jul 25, 2024

srl295 commented Jul 31, 2024

srl295 left a comment

Choose a reason for hiding this comment

srl295 commented Jul 31, 2024 • edited Loading

catamorphism commented Aug 1, 2024

catamorphism commented Aug 1, 2024 • edited Loading

catamorphism commented Aug 1, 2024 • edited Loading

echeran left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Aug 7, 2024

srl295 left a comment

Choose a reason for hiding this comment

markusicu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catamorphism commented Jun 26, 2024 •

edited

Loading

srl295 Jun 28, 2024 •

edited

Loading

catamorphism commented Jul 17, 2024 •

edited

Loading

srl295 commented Jul 31, 2024 •

edited

Loading

catamorphism commented Aug 1, 2024 •

edited

Loading

catamorphism commented Aug 1, 2024 •

edited

Loading