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.

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

test(core): verify Unicode and ICU versions cross-platform 🙀 #11418

Merged
merged 5 commits into from
May 27, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented May 11, 2024

  • load versions from node and ICU4C++ and Blocks.txt
  • validate Node version against package.json's engine
  • for now, don't complain if ICU4C++ mismatches, but DO check Unicode versions.

Fixes: #10183

@keymanapp-test-bot skip

@srl295 srl295 self-assigned this May 11, 2024
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S2 milestone May 11, 2024
@github-actions github-actions bot added the core/ Keyman Core label May 11, 2024
@srl295 srl295 changed the title Chore/common/10183-unicode-version chore(core): test for Unicode version 🙀 May 11, 2024
@srl295
Copy link
Member Author

srl295 commented May 11, 2024

Example output

= test_all
== load_json loading tests/unit/ldml/nodeversions.json
== load_json loading ../../../../package.json
== test_unicode_versions
cxx_icu	73.1
cxx_icu_unicode	15.0
node_icu_unicode	15.0
node	18.16.0
node_icu	72.1
node_engine	^18.x
block_unicode_ver	15.1.0.txt

@srl295
Copy link
Member Author

srl295 commented May 13, 2024

So linux is using ICU from the environment, which only supports Unicode 14 not 15.

  stdout:
20:08:09   = test_all
20:08:09   == load_json loading tests/unit/ldml/nodeversions.json
20:08:09   == load_json loading ../../../../package.json
20:08:09   == test_unicode_versions
20:08:09   cxx_icu  70.1
20:08:09   cxx_icu_unicode  14.0
20:08:09   node_icu_unicode  15.0
20:08:09   node  18.17.1
20:08:09   node_icu  73.1
20:08:09   node_engine  ^18.x
20:08:09   block_unicode_ver  15.1.0.txt
20:08:09   stderr:
20:08:09   Test failed at ../../../tests/unit/ldml/test_unicode.cpp:110:
20:08:09   expected: 14
20:08:09   actual:   15

@srl295 srl295 changed the base branch from master to chore/core/8800-cxx17 May 13, 2024 23:14
@srl295 srl295 force-pushed the chore/common/10183-unicode-version branch from c7b51ba to d3f8e07 Compare May 13, 2024 23:16
@github-actions github-actions bot added the chore label May 13, 2024
@srl295
Copy link
Member Author

srl295 commented May 13, 2024

So linux is using ICU from the environment, which only supports Unicode 14 not 15.

  stdout:
20:08:09   = test_all
20:08:09   == load_json loading tests/unit/ldml/nodeversions.json
20:08:09   == load_json loading ../../../../package.json
20:08:09   == test_unicode_versions
20:08:09   cxx_icu  70.1
20:08:09   cxx_icu_unicode  14.0
20:08:09   node_icu_unicode  15.0
20:08:09   node  18.17.1
20:08:09   node_icu  73.1
20:08:09   node_engine  ^18.x
20:08:09   block_unicode_ver  15.1.0.txt
20:08:09   stderr:
20:08:09   Test failed at ../../../tests/unit/ldml/test_unicode.cpp:110:
20:08:09   expected: 14
20:08:09   actual:   15

I'm going to take out the assert for the cxx_icu version, for now.

Base automatically changed from chore/core/8800-cxx17 to master May 14, 2024 03:35
@srl295
Copy link
Member Author

srl295 commented May 14, 2024

all working but wasm

- load version data from node.js, Blocks.txt, and ICU4C
- support wasm: copy package.json, nodeversions.json and Blocks.txt into the keyboard area so that they can be mounted under wasm

also:
- rename 'fallback' macro to KMN_FALLBACK to not conflict with hedley in utfcodec.hpp
- fix ambiguous path type in tests

Fixes: #10183
@srl295 srl295 force-pushed the chore/common/10183-unicode-version branch from 490b510 to e5ee26a Compare May 23, 2024 14:33
@srl295 srl295 changed the title chore(core): test for Unicode version 🙀 test(core): verify Unicode and ICU versions cross-platform 🙀 May 23, 2024
@srl295 srl295 added test Any acceptance test issue and removed chore labels May 23, 2024
- incorporate change from #11483

Fixes: #10183
@srl295 srl295 marked this pull request as ready for review May 23, 2024 22:44
@srl295 srl295 requested review from mcdurdin and rc-swag as code owners May 23, 2024 22:44
@srl295
Copy link
Member Author

srl295 commented May 23, 2024

OK ptal, web unrelated?

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

Looks fine, just small nits

# Build ldml test executable

keyboard_build_path = join_paths(meson.current_build_dir(),'keyboards')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
keyboard_build_path = join_paths(meson.current_build_dir(),'keyboards')
keyboard_build_path = meson.current_build_dir() / 'keyboards'

It's cleaner to use / now in meson path construction

@@ -94,14 +120,34 @@ if cpp_compiler.get_id() == 'emscripten'
normalization_tests_flags += ['-lnodefs.js', '-sEXPORTED_RUNTIME_METHODS=[\'UTF8ToString\']']
endif

t = executable('test_context_normalization',
tc = executable('test_context_normalization',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tc = executable('test_context_normalization',
test_context_normalization = executable('test_context_normalization',

Can we use the full process name as the variable name? Makes future maintenance so much easier


# Build and run additional test_unicode test

u = executable('test_unicode', 'test_unicode.cpp',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
u = executable('test_unicode', 'test_unicode.cpp',
test_unicode = executable('test_unicode', 'test_unicode.cpp',

Ditto

@srl295 srl295 requested a review from mcdurdin May 24, 2024 03:19
@mcdurdin mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024
@mcdurdin
Copy link
Member

@ermshiperete the API Verification build is failing, any ideas why? Log is inconclusive.

@ermshiperete
Copy link
Contributor

@ermshiperete the API Verification build is failing, any ideas why? Log is inconclusive.

Ignore the API Verification for now. I thought it would work, but I must be overlooking something and it's still not working. I'll add a step to always greenbar until that's solved. Sorry for the noise.

@ermshiperete
Copy link
Contributor

Example output

= test_all
== load_json loading tests/unit/ldml/nodeversions.json
== load_json loading ../../../../package.json
== test_unicode_versions
cxx_icu	73.1
cxx_icu_unicode	15.0
node_icu_unicode	15.0
node	18.16.0
node_icu	72.1
node_engine	^18.x
block_unicode_ver	15.1.0.txt

I'm wondering if it would be better to change the order of the output so that all ICU versions are together, all Unicode versions are together etc. ? That way it would be easier to spot which versions should be the same.

@srl295
Copy link
Member Author

srl295 commented May 27, 2024

Example output

= test_all
== load_json loading tests/unit/ldml/nodeversions.json
== load_json loading ../../../../package.json
== test_unicode_versions
cxx_icu	73.1
cxx_icu_unicode	15.0
node_icu_unicode	15.0
node	18.16.0
node_icu	72.1
node_engine	^18.x
block_unicode_ver	15.1.0.txt

I'm wondering if it would be better to change the order of the output so that all ICU versions are together, all Unicode versions are together etc. ? That way it would be easier to spot which versions should be the same.

Good idea. Maybe with some explanatory text, even.

I think I'll do this as a separate PR since this stack is getting longer.

@srl295 srl295 merged commit 1700953 into master May 27, 2024
17 of 18 checks passed
@srl295 srl295 deleted the chore/common/10183-unicode-version branch May 27, 2024 13:25
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.45-alpha

@srl295
Copy link
Member Author

srl295 commented May 27, 2024

Example output

= test_all
== load_json loading tests/unit/ldml/nodeversions.json
== load_json loading ../../../../package.json
== test_unicode_versions
cxx_icu	73.1
cxx_icu_unicode	15.0
node_icu_unicode	15.0
node	18.16.0
node_icu	72.1
node_engine	^18.x
block_unicode_ver	15.1.0.txt

I'm wondering if it would be better to change the order of the output so that all ICU versions are together, all Unicode versions are together etc. ? That way it would be easier to spot which versions should be the same.

#11572

@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.45-alpha

5 similar comments
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.45-alpha

@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.45-alpha

@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.45-alpha

@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.45-alpha

@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.45-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/ Keyman Core test Any acceptance test issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(core): unit test to verify cross-project Unicode versions 🙀
4 participants