feat(core): ldml marker normalization 🙀 #9761

srl295 · 2023-10-13T22:00:24Z

add some failing tests
fix them

For: #9468

@keymanapp-test-bot skip

keymanapp-test-bot · 2023-10-13T22:00:30Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 17.0.215 (0.9761.9798)
Keyboards
- Test Keyboards
Linux
- Keyman for Linux
macOS
- Keyman for macOS
Web
- KeymanWeb Test Home
Windows

srl295 · 2023-11-02T22:49:51Z

moved the failing bengali test case here for fixin'!

mcdurdin · 2023-11-03T23:19:17Z

I think we need to bring up to date with master in order for some of the Linux builds to work.

mcdurdin

Oh my C++ Unicode escapes are so hard to read. Suggestions added

core/tests/unit/ldml/test_transforms.cpp

mcdurdin · 2023-11-08T22:52:40Z

#9761 had 4 failures, Windows x 2 were:

02:37:59   ../../../src/debuglog.cpp(467): error C2220: the following warning is treated as an error
02:37:59   ../../../src/debuglog.cpp(467): warning C4477: 'snprintf' : format string '%4.6X' requires an argument of type 'unsigned int', but variadic argument 1 has type 'char32_t'

https://build.palaso.org/buildConfiguration/KeymanDesktop_TestPullRequests/422546?buildTab=log&focusLine=2451&linesState=370&logView=flowAware

and macOS:

73/83 keyman_core:ldml-keyboards / k_008_transform_norm        FAIL            0.06s   killed by signal 6 SIGABRT
...
83/83 keyman_core:ldml / test_transforms                       FAIL            0.81s   killed by signal 6 SIGABRT

https://build.palaso.org/buildConfiguration/Keyman_Common_KPAPI_TestPullRequests_macOS/422542?buildTab=log&focusLine=3094&linesState=89&logView=flowAware

Developer build failure was codesigning fail. Will restart that one but it will probably fail on Core?

srl295 · 2023-11-09T02:00:20Z

Hm ok Thanks El El mié, nov. 8, 2023 a la(s) 4:52 p.m., Marc Durdin < ***@***.***> escribió:

…

#9761 <#9761> had 4 failures, Windows x 2 were: 02:37:59 ../../../src/debuglog.cpp(467): error C2220: the following warning is treated as an error 02:37:59 ../../../src/debuglog.cpp(467): warning C4477: 'snprintf' : format string '%4.6X' requires an argument of type 'unsigned int', but variadic argument 1 has type 'char32_t' https://build.palaso.org/buildConfiguration/KeymanDesktop_TestPullRequests/422546?buildTab=log&focusLine=2451&linesState=370&logView=flowAware and macOS: 73/83 keyman_core:ldml-keyboards / k_008_transform_norm FAIL 0.06s killed by signal 6 SIGABRT ... 83/83 keyman_core:ldml / test_transforms FAIL 0.81s killed by signal 6 SIGABRT https://build.palaso.org/buildConfiguration/Keyman_Common_KPAPI_TestPullRequests_macOS/422542?buildTab=log&focusLine=3094&linesState=89&logView=flowAware Developer build failure was codesigning fail. Will restart that one but it will probably fail on Core? — Reply to this email directly, view it on GitHub <#9761 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGQZMZKE3TKK4U34BUD2ATYDQEMHAVCNFSM6AAAAAA57XCBZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBSHAZDOMZVGE> . You are receiving this because you were assigned.Message ID: ***@***.***>

srl295 · 2023-11-09T03:53:17Z

put in a patch

srl295 · 2023-11-11T00:27:06Z

@mcdurdin fail didn't seem to repro here, but it turns out:

but

../../../tests/unit/ldml/ldml_test_source.cpp:735: warning: (!rep_tests.empty()) && (rep_tests.size() > 0)
\x1b[38;5;196mTest failed with 0 at ../../../tests/unit/ldml/ldml.cpp:192:\x1b[0m
  ci->type != KM_CORE_CT_END && test_ci != test_context.end()

Seems like the status isn't being reported properly. So i'm getting false positives on my end.

EDIT Whoa, "test failed with 0" … but, that ends up std::exit(result * 100) i.e. exit(0) = success!

Hmm.

srl295 · 2023-11-11T00:33:37Z

@ermshiperete @mcdurdin std::exit(0) was being called on test failure… take a look at the test_assert.h change

…e/9468-marker-normalization-epic-ldml

srl295 · 2023-11-14T13:32:54Z

I cleaned this up and moved the test-case-fix stuff into another PR

- go back to NFD for the context, for now - anticipating when the privatecontext is NFD but the public context is NFC - also update the test cases For: #9468

- a little further - couple places where "it wasn't plugged in" - adding some LDML-TODOs - marker creep - fixed one unnecessary alloc/dealloc For: #9468

- temporarily remove a backspace test until #9450 is fixed. For: #9468

- test fix For: #9468

mcdurdin · 2023-11-16T08:40:18Z

Keyman Core macOS build failed with:

Assertion failed: (*i == LDML_MARKER_CODE), function remove_markers, file ldml_transforms.cpp, line 1007.

srl295 · 2023-11-16T16:59:17Z

doesn't repro locally, checking still

- literally a bad assert. the error case is handled below, in fact the unit test tests for it. - for some reason, assert.h wasn't included in some cases locally. For: #9468

srl295 · 2023-11-16T20:58:49Z

doesn't repro locally, checking still

fixed, it was actually a bad assert. The case it was asserting for was handled in error checking code one line below. I had a local build environment that ended up ignoring the assert (which was a source bug also fixed)!

mcdurdin

LGTM as far as it goes -- still more iterations needed on multiple markers between two chars I guess.

One issue possible with BT_UNKNOWN type backspaces -- it should only happen with an empty context.

mcdurdin · 2023-11-19T09:12:24Z

core/tests/unit/ldml/ldml.cpp

+        assert(context.back().character == ch);
+        context.pop_back();
+      } else {
+        assert(act.backspace.expected_type == KM_CORE_BT_UNKNOWN); // else it must be unknown


BT_UNKNOWN should only be used when the context is empty.

added an assert for that request

- KM_CORE_BT_UNKNOWN should only be used with an empty context For: #9468 but related to #9450

srl295 · 2023-11-20T23:17:12Z

might as well update. mac failure seemed to be timeout

keyman-server · 2023-11-22T18:08:29Z

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

srl295 self-assigned this Oct 13, 2023

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Oct 13, 2023

keymanapp-test-bot bot added this to the A17S23 milestone Oct 13, 2023

srl295 changed the title ~~feat(core): ldml normalization 🙀~~ feat(core): ldml marker normalization 🙀 Oct 13, 2023

github-actions bot added core/ Keyman Core feat labels Oct 13, 2023

mcdurdin modified the milestones: A17S23, A17S24 Oct 15, 2023

keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Oct 19, 2023

Base automatically changed from feat/core/9468-test-improvement-epic-ldml to master October 20, 2023 03:58

mcdurdin modified the milestones: A17S24, A17S25 Oct 27, 2023

srl295 force-pushed the feat/core/9468-marker-normalization-epic-ldml branch from 247ce11 to 8796b50 Compare November 2, 2023 22:49

srl295 changed the base branch from master to chore/developer/9838-cldr44-moredata-epic-ldml November 2, 2023 22:49

github-actions bot added common/ common/resources/ Build infrastructure labels Nov 2, 2023

Base automatically changed from chore/developer/9838-cldr44-moredata-epic-ldml to master November 3, 2023 17:29

mcdurdin reviewed Nov 6, 2023

View reviewed changes

core/tests/unit/ldml/test_transforms.cpp Outdated Show resolved Hide resolved

core/tests/unit/ldml/test_transforms.cpp Outdated Show resolved Hide resolved

srl295 marked this pull request as ready for review November 7, 2023 16:39

srl295 requested review from jahorton and rc-swag as code owners November 7, 2023 16:39

srl295 requested a review from ermshiperete November 11, 2023 00:33

srl295 changed the base branch from master to fix/common/10004-truthful-test-epic-ldml November 14, 2023 13:29

Merge branch 'fix/common/10004-truthful-test-epic-ldml' into feat/cor…

bf66909

…e/9468-marker-normalization-epic-ldml

feat(core): marker normalization 🙀

2f843d9

- go back to NFD for the context, for now - anticipating when the privatecontext is NFD but the public context is NFC - also update the test cases For: #9468

Base automatically changed from fix/common/10004-truthful-test-epic-ldml to master November 15, 2023 06:37

srl295 added 4 commits November 15, 2023 00:37

Merge branch 'master' into feat/core/9468-marker-normalization-epic-ldml

0841afb

feat(core): marker normalization 🙀

07a4821

- a little further - couple places where "it wasn't plugged in" - adding some LDML-TODOs - marker creep - fixed one unnecessary alloc/dealloc For: #9468

feat(core): marker normalization 🙀

5a04a81

- temporarily remove a backspace test until #9450 is fixed. For: #9468

Merge branch 'master' into feat/core/9468-marker-normalization-epic-ldml

2163c1d

srl295 marked this pull request as ready for review November 15, 2023 23:10

srl295 marked this pull request as draft November 15, 2023 23:11

feat(core): marker normalization 🙀

e28f359

- test fix For: #9468

srl295 marked this pull request as ready for review November 15, 2023 23:29

Merge branch 'master' into feat/core/9468-marker-normalization-epic-ldml

3905c6d

srl295 marked this pull request as draft November 16, 2023 17:53

srl295 added 2 commits November 16, 2023 12:11

feat(core): marker normalization 🙀

4bbafa6

- literally a bad assert. the error case is handled below, in fact the unit test tests for it. - for some reason, assert.h wasn't included in some cases locally. For: #9468

Merge branch 'master' into feat/core/9468-marker-normalization-epic-ldml

386ac30

srl295 marked this pull request as ready for review November 16, 2023 21:37

Merge branch 'master' into feat/core/9468-marker-normalization-epic-ldml

934dd45

mcdurdin approved these changes Nov 19, 2023

View reviewed changes

srl295 added 2 commits November 20, 2023 16:57

feat(developer): ldml fix testcase processing 🙀

f524519

- KM_CORE_BT_UNKNOWN should only be used with an empty context For: #9468 but related to #9450

Merge branch 'master' into feat/core/9468-marker-normalization-epic-ldml

5f6dafa

srl295 merged commit 850491c into master Nov 21, 2023
25 checks passed

srl295 deleted the feat/core/9468-marker-normalization-epic-ldml branch November 21, 2023 21:01

mcdurdin added the epic-core/9999-normalization label Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): ldml marker normalization 🙀 #9761

feat(core): ldml marker normalization 🙀 #9761

srl295 commented Oct 13, 2023 •

edited

Loading

keymanapp-test-bot bot commented Oct 13, 2023 •

edited

Loading

srl295 commented Nov 2, 2023

mcdurdin commented Nov 3, 2023

mcdurdin left a comment

mcdurdin commented Nov 8, 2023

srl295 commented Nov 9, 2023 via email

srl295 commented Nov 9, 2023

srl295 commented Nov 11, 2023 •

edited

Loading

srl295 commented Nov 11, 2023

srl295 commented Nov 14, 2023

mcdurdin commented Nov 16, 2023

srl295 commented Nov 16, 2023

srl295 commented Nov 16, 2023 •

edited

Loading

mcdurdin left a comment

mcdurdin Nov 19, 2023

srl295 Nov 20, 2023

srl295 commented Nov 20, 2023

keyman-server commented Nov 22, 2023

feat(core): ldml marker normalization 🙀 #9761

feat(core): ldml marker normalization 🙀 #9761

Conversation

srl295 commented Oct 13, 2023 • edited Loading

keymanapp-test-bot bot commented Oct 13, 2023 • edited Loading

User Test Results

Test Artifacts

srl295 commented Nov 2, 2023

mcdurdin commented Nov 3, 2023

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin commented Nov 8, 2023

srl295 commented Nov 9, 2023 via email

srl295 commented Nov 9, 2023

srl295 commented Nov 11, 2023 • edited Loading

srl295 commented Nov 11, 2023

srl295 commented Nov 14, 2023

mcdurdin commented Nov 16, 2023

srl295 commented Nov 16, 2023

srl295 commented Nov 16, 2023 • edited Loading

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Nov 19, 2023

Choose a reason for hiding this comment

srl295 Nov 20, 2023

Choose a reason for hiding this comment

srl295 commented Nov 20, 2023

keyman-server commented Nov 22, 2023

srl295 commented Oct 13, 2023 •

edited

Loading

keymanapp-test-bot bot commented Oct 13, 2023 •

edited

Loading

srl295 commented Nov 11, 2023 •

edited

Loading

srl295 commented Nov 16, 2023 •

edited

Loading