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鈥檒l 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
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

Fixes: #10183

Co-authored-by: Marc Durdin <marc@durdin.net>
@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
Development

Successfully merging this pull request may close these issues.

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