test(developer): kmcmplib compiler unit tests 2 #11663

markcsinclair · 2024-06-03T11:00:28Z

Google Test based unit tests for Compiler.cpp

Continues PR #11378

@keymanapp-test-bot skip

keymanapp-test-bot · 2024-06-03T11:00:31Z

User Test Results

Test specification and instructions

User tests are not required

markcsinclair · 2024-06-08T12:23:38Z

Hmm ... one of the commits (54d82b5) says initFileKeyboard when it should say deleteFileKeyboard ... too late to change.

…compiler-unit-tests-2 # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

…compiler-unit-tests-2

…compiler-unit-tests-2 # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

…pl_type_a test

…ndex test case

…mpl_type_b baselayout test

…mpl_type_i if test

…mpl_type_o test

markcsinclair · 2024-07-08T11:15:46Z

Putting this in for review as it stands, and will continue in a new PR. There are several test cases written but commented out due to issues #11814 (u16tok() bug)and #11937 (GetDelimitedString() bug), as well as a test that passes and shouldn't due to issue #11833 (a space works as well as a comma between arguments in index()). The fixes for the outstanding issues should uncomment (or reverse the outcome) of the relevant test cases here.

…compiler-unit-tests-2 # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

mcdurdin

As far as I can tell, all the tests look good.

The compiler code is pretty yeuch but this is at least pushing stability in the right direction. If we have a comprehensive set of tests, we can rewrite bits one day!

jahorton

LGTM outside of lines 876 - 879.

I'm not currently able to adequately parse the setup for that block to know why the test works and makes sense. It's not that I found an issue; it's more that I'm having difficulty parsing what it represents and so cannot give good feedback there. Feel free to move forward if you're not too worried about that particular test - it's one of many, and the reset seem fine and clear enough.

…compiler-unit-tests-2 # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

… tests now fixes are available

…tsTooLong test case

markcsinclair · 2024-07-23T19:31:00Z

LGTM outside of lines 876 - 879.

I'm not currently able to adequately parse the setup for that block to know why the test works and makes sense. It's not that I found an issue; it's more that I'm having difficulty parsing what it represents and so cannot give good feedback there. Feel free to move forward if you're not too worried about that particular test - it's one of many, and the reset seem fine and clear enough.

I have added some local variables and comments to try to clarify the CERR_OutsTooLong test case

jahorton · 2024-07-24T03:08:02Z

developer/src/kmcmplib/tests/gtest-compiler-test.cpp

+    // outs, CERR_OutsTooLong
+    PKMX_WCHAR dpString = (PKMX_WCHAR)u"abc";
+    file_store[1].dpString = dpString; // length 4 => max should be > 4, otherwise a CERR_OutsTooLong is emitted
+    int max = u16len(dpString) + 1; // 4, including terminating '\0'
+    u16cpy(str, u"outs(b)");
+    EXPECT_EQ(CERR_OutsTooLong, GetXStringImpl(tstr, &fileKeyboard, str, u"", output, max, 0, &newp, FALSE)); // max reduced to force error


So, I realize that the code being tested isn't yours... but man, that is not an intuitive pattern for max. In the worst case, I'd expect max=4 to be perfectly fine with 3 chars + 1 null terminator. 4 is within a max of 4, after all. (I can understand not making the null terminator implicit due to how C / C++ strings work.)

Sure. Looks like an off-by-one in calculation, but it's off-by-one in a conservative way so not really any harm done. If you are looking for intuitive code, I suggest you don't look at the compiler.

Interestingly, it is one of the very few places in the kmcmplib compiler where any of the code tests to see if there is room in the buffer before writing (see here). Buffer overruns are detected post facto by Marc's high-level fix here.

Yeah, so many problems with buffer management. Yay C. My high-level fix is an attempt to mitigate rather than resolve, as we slowly inch towards a rewrite one day.

markcsinclair · 2024-07-24T10:25:52Z

Re-review requested as nine test cases uncommented and passing due to having merged #11938 and #11910 into master.

jahorton

I'm admittedly not terribly clear on the cases from among the newly-uncommented nine (see ea0a2f7) that involve the special Keyman-language keyword beep in the midst of other statements, but otherwise this LGTM.

markcsinclair · 2024-07-26T10:18:40Z

I'm admittedly not terribly clear on the cases from among the newly-uncommented nine (see ea0a2f7) that involve the special Keyman-language keyword beep in the midst of other statements, but otherwise this LGTM.

The reason the beeps are there is because the compiler renenters on that part of the statement (e.g. if you call GetXStringImpl() on u"baselayout(beep)", the call chain includes process_baselayout() on the argument beep, which ultimately calls GetXStringImpl() on beep. I selected beep as a simple command that I had already tested. In an ideal world, all of these case statements in GetXStringImpl() and the functions they call would be stubbable virtual class methods, so the unit test didn't extend beyond a single method, but what can you do!

…compiler-unit-tests-2 # Keyman Conventional Commit suggestions: # # - Link to a Sentry issue with git trailer: # Fixes: _MODULE_-_ID_ # - Give credit to co-authors: # Co-authored-by: _Name_ <_email_> # - Use imperative, present tense ('attach' not 'attaches', 'attached' etc) # - Don't include a period at the end of the title # - Always include a blank line before trailers # - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes

keyman-server · 2024-07-26T18:03:04Z

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

markcsinclair requested review from mcdurdin and darcywong00 as code owners June 3, 2024 11:00

keymanapp-test-bot bot added this to the A18S3 milestone Jun 3, 2024

github-actions bot added developer/ developer/compilers/ test Any acceptance test issue labels Jun 3, 2024

markcsinclair self-assigned this Jun 3, 2024

markcsinclair marked this pull request as draft June 3, 2024 11:01

markcsinclair changed the title ~~test(developer) kmcmplib compiler unit tests 2~~ test(developer): kmcmplib compiler unit tests 2 Jun 7, 2024

mcdurdin modified the milestones: A18S3, A18S4 Jun 7, 2024

This was referenced Jun 10, 2024

test(developer): kmcmplib compiler unit tests #11378

Merged

fix(developer): make native compilation of kmcmplib under Linux possible #11779

Merged

markcsinclair added 3 commits June 18, 2024 14:06

chore(developer): delete index test cases now covered in #11815

1d47042

chore(developer): add deletes to clean up file_store

9a2eb59

chore(developer): remove duplicate deletes

6194911

markcsinclair mentioned this pull request Jun 19, 2024

fix(developer): handle second parameter of index correctly in kmcmplib compiler #11815

Merged

darcywong00 modified the milestones: A18S4, A18S5 Jun 21, 2024

markcsinclair added 3 commits June 24, 2024 10:07

chore(developer): Merge branch 'master' into test/developer/kmcmplib-…

7285379

…compiler-unit-tests-2

darcywong00 modified the milestones: A18S5, A18S6 Jul 8, 2024

markcsinclair added 4 commits July 8, 2024 09:56

chore(developer): add additional whitspace test cases to GetXStringIm…

1b6f9de

…pl_type_a test

chore(developer): clarify comments, replace space with comma in one i…

4271a84

…ndex test case

chore(developer): add additional whitespace test cases to GetXStringI…

340d429

…mpl_type_b baselayout test

markcsinclair added 2 commits July 8, 2024 11:45

chore(developer): add additional whitespace test cases to GetXStringI…

60d6797

…mpl_type_i if test

chore(developer): add additional whitespace test cases to GetXStringI…

017d01f

…mpl_type_o test

markcsinclair marked this pull request as ready for review July 8, 2024 11:16

markcsinclair mentioned this pull request Jul 9, 2024

fix(developer): correct handling of trailing spaces by GetDelimitedString() in kmcmplib compiler #11938

Merged

darcywong00 modified the milestones: A18S6, A18S7 Jul 19, 2024

markcsinclair mentioned this pull request Jul 22, 2024

test(developer): kmcmplib compiler unit tests 3 #11990

Draft

mcdurdin approved these changes Jul 23, 2024

View reviewed changes

jahorton approved these changes Jul 23, 2024

View reviewed changes

markcsinclair added 4 commits July 23, 2024 11:59

chore(developer): uncomment nine test cases in various GetXStringImpl…

ea0a2f7

… tests now fixes are available

chore(developer): add local variables and comments to clarify CERR_Ou…

c0524ab

…tsTooLong test case

jahorton reviewed Jul 24, 2024

View reviewed changes

markcsinclair requested a review from mcdurdin July 24, 2024 10:26

mcdurdin requested a review from jahorton July 25, 2024 01:50

jahorton approved these changes Jul 25, 2024

View reviewed changes

markcsinclair merged commit 0f7d0c5 into master Jul 26, 2024
4 checks passed

markcsinclair deleted the test/developer/kmcmplib-compiler-unit-tests-2 branch July 26, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(developer): kmcmplib compiler unit tests 2 #11663

test(developer): kmcmplib compiler unit tests 2 #11663

markcsinclair commented Jun 3, 2024

keymanapp-test-bot bot commented Jun 3, 2024 •

edited

Loading

markcsinclair commented Jun 8, 2024

markcsinclair commented Jul 8, 2024 •

edited

Loading

mcdurdin left a comment

jahorton left a comment

markcsinclair commented Jul 23, 2024

jahorton Jul 24, 2024

mcdurdin Jul 24, 2024

markcsinclair Jul 24, 2024

mcdurdin Jul 25, 2024 •

edited

Loading

markcsinclair commented Jul 24, 2024 •

edited

Loading

jahorton left a comment •

edited

Loading

markcsinclair commented Jul 26, 2024

keyman-server commented Jul 26, 2024

test(developer): kmcmplib compiler unit tests 2 #11663

test(developer): kmcmplib compiler unit tests 2 #11663

Conversation

markcsinclair commented Jun 3, 2024

keymanapp-test-bot bot commented Jun 3, 2024 • edited Loading

User Test Results

markcsinclair commented Jun 8, 2024

markcsinclair commented Jul 8, 2024 • edited Loading

mcdurdin left a comment

Choose a reason for hiding this comment

jahorton left a comment

Choose a reason for hiding this comment

markcsinclair commented Jul 23, 2024

jahorton Jul 24, 2024

Choose a reason for hiding this comment

mcdurdin Jul 24, 2024

Choose a reason for hiding this comment

markcsinclair Jul 24, 2024

Choose a reason for hiding this comment

mcdurdin Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

markcsinclair commented Jul 24, 2024 • edited Loading

jahorton left a comment • edited Loading

Choose a reason for hiding this comment

markcsinclair commented Jul 26, 2024

keyman-server commented Jul 26, 2024

keymanapp-test-bot bot commented Jun 3, 2024 •

edited

Loading

markcsinclair commented Jul 8, 2024 •

edited

Loading

mcdurdin Jul 25, 2024 •

edited

Loading

markcsinclair commented Jul 24, 2024 •

edited

Loading

jahorton left a comment •

edited

Loading