-
-
Notifications
You must be signed in to change notification settings - Fork 107
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(developer): kmcmplib compiler unit tests 2 #11663
test(developer): kmcmplib compiler unit tests 2 #11663
Conversation
User Test ResultsTest specification and instructions User tests are not required |
Hmm ... one of the commits (54d82b5) says |
…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 # 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
…mpl_type_b baselayout test
…mpl_type_i if test
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 ( |
…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 # 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…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
I have added some local variables and comments to try to clarify the |
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
…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
Changes in this pull request will be available for download in Keyman version 18.0.76-alpha |
Google Test based unit tests for Compiler.cpp
Continues PR #11378
@keymanapp-test-bot skip