-
Notifications
You must be signed in to change notification settings - Fork 131
filtering out non-BMP characters from CJK string #20514
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
base: master
Are you sure you want to change the base?
filtering out non-BMP characters from CJK string #20514
Conversation
|
|
PRT Result |
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.
Hey there - I've reviewed your changes - here's some feedback:
- The CJK regeneration loop in
filtered_datapointuses hard-coded values formin_length(half the original length) andmax_attempts(10); consider making these configurable or at least defining them as named constants to clarify the intended behavior and make future tuning easier. - If all regeneration attempts fail and the fallback
gen_string('cjk', 10)still produces mostly non-BMP characters,dataset['cjk']may end up unexpectedly short; you might want to explicitly enforce a minimum length after the fallback or log when the code cannot generate a sufficiently long BMP-only CJK string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CJK regeneration loop in `filtered_datapoint` uses hard-coded values for `min_length` (half the original length) and `max_attempts` (10); consider making these configurable or at least defining them as named constants to clarify the intended behavior and make future tuning easier.
- If all regeneration attempts fail and the fallback `gen_string('cjk', 10)` still produces mostly non-BMP characters, `dataset['cjk']` may end up unexpectedly short; you might want to explicitly enforce a minimum length after the fallback or log when the code cannot generate a sufficiently long BMP-only CJK string.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jnagare-redhat
left a comment
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
|
I'd recommend implementing CJK generation in Fauxfactory without BMP characters first, and then follow up here in Robottelo. |
vsedmik
left a comment
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.
Looks like @omaciel already implemented the change in fauxfactory 🚀
Could we use it, @vijaysawant ?
Problem Statement
test case
test_positive_create_in_different_orgsfailing for cjk parameterised string with an errorselenium.common.exceptions.WebDriverException: Message: unknown error: ChromeDriver only supports characters in the BMPSolution
filtering out non-BMP characters is the recommended way instead of removing CJK from
valid_data_list()Related Issues
PRT test Cases example