icu icon indicating copy to clipboard operation
icu copied to clipboard

ICU-22746 Refactor some MF2 tests to be data-driven

Open catamorphism opened this issue 10 months ago • 4 comments

This change moves all test strings out of test/intltest/messageformat2test.cpp and into JSON files, which are parsed/run by code in a new file, test/intltest/messageformat2test_read_json.cpp . It also removes the file test/intltest/messageformat2test_fromjson.cpp , which contained tests that are now stored in JSON files.

Some of the tests in other files will need to be converted; this is an incremental change.

A new vendored library, https://github.com/nlohmann/json/ , is added as a dependency. This accounts for most of the "files changed".

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

catamorphism avatar Apr 22 '24 19:04 catamorphism

cc @echeran -- I didn't fix the copyright scan errors since you mentioned you would help with legal signoff re: licensing for the nlohmann/json library.

cc @srl295 @roubert -- I tried to follow what you suggested with vendoring the library; let me know if this is what you had in mind. I blatantly copy-pasted the pull-from-upstream.sh and UPDATING.md files from the vendor/double-conversion directory, but if you think it's worth generalizing the instructions and shell script, let me know.

catamorphism avatar Apr 23 '24 21:04 catamorphism

good work and a couple of comments

  • i see two copies of ujson.h and probably only need one
  • does the entire repo need to be vendored since only one file is used? perhaps only the .h file and the license?
  • it might be good to have a file with the latest hash and version or such (in the vendor directory)

srl295 avatar Apr 23 '24 22:04 srl295

good work and a couple of comments

  • i see two copies of ujson.h and probably only need one

Thanks for spotting that, fixed.

  • does the entire repo need to be vendored since only one file is used? perhaps only the .h file and the license?

Probably not, no. Changed this in 24de7c4 and removed the extra files.

  • it might be good to have a file with the latest hash and version or such (in the vendor directory)

Do you mean a manually updated file, or changing the pull-from-upstream.sh script to update the file when it's run?

catamorphism avatar Apr 23 '24 23:04 catamorphism

  • [ ] also this PR needs to update the top level LICENSE file to include a copy of LICENSE.MIT, see other examples
  • [ ] cpyscan tool can exclude .json files and the vendor/ directory.

srl295 avatar May 01 '24 17:05 srl295

I double checked with Google's legal counsel for open source, who gave approval continent on guidelines that already match ICU's current processes for vendoring 3rd party sources:

It will be fine to vendor MIT-licensed code into ICU. ... The LICENSE file that lives alongside the nlohmann/json code in the /vendor subdirectory of ICU needs to inherit the specific MIT license file at https://github.com/nlohmann/json/blob/develop/LICENSE.MIT with its upstream copyright notice intact.

echeran avatar May 03 '24 22:05 echeran

  • should have some mention of checking that the license doesn't change

I think your suggested change, which I incorporated, addresses this?

catamorphism avatar May 06 '24 20:05 catamorphism

OK, I think I've incorporated all the licensing changes you suggested, @echeran and @srl295 -- can you look again? Thanks!

catamorphism avatar May 06 '24 21:05 catamorphism

@catamorphism all LGTM!

  • probably want to squash it per ICU prefs - https://us-central1-dev-infra-273822.cloudfunctions.net/unicode-github-bot/info/unicode-org/icu/2980 (the 'Details' under the single-commit check) will do it for you
  • @echeran are you driving the ticket and the merge?

srl295 avatar May 06 '24 22:05 srl295

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

~ Your Friendly Jira-GitHub PR Checker Bot

Thanks! I've squashed.

catamorphism avatar May 07 '24 20:05 catamorphism