Skip to content

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

Merged
srl295 merged 5 commits intomasterfrom
chore/common/10183-unicode-version
May 27, 2024
Merged

test(core): verify Unicode and ICU versions cross-platform 🙀 #11418
srl295 merged 5 commits intomasterfrom
chore/common/10183-unicode-version

Conversation

@srl295
Copy link
Copy Markdown
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
Copy link
Copy Markdown

keymanapp-test-bot bot commented 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

srl295 commented May 23, 2024

OK ptal, web unrelated?

Copy link
Copy Markdown
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
Copy Markdown
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

endif

t = executable('test_context_normalization',
tc = executable('test_context_normalization',
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@ermshiperete
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@srl295 srl295 deleted the chore/common/10183-unicode-version branch May 27, 2024 13:25
@keyman-server
Copy link
Copy Markdown
Collaborator

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

@srl295
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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

@keyman-server
Copy link
Copy Markdown
Collaborator

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

@keyman-server
Copy link
Copy Markdown
Collaborator

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

@keyman-server
Copy link
Copy Markdown
Collaborator

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

@keyman-server
Copy link
Copy Markdown
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