Skip to content
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

String tests #179

Merged
merged 6 commits into from
Dec 8, 2016
Merged

String tests #179

merged 6 commits into from
Dec 8, 2016

Conversation

mikaelarguedas
Copy link
Member

This PR adds:

  • [rosidl_generator_py]
    • length enforcement for bounded string in array in Python and the relevant unit tests
  • [rosidl_generator_cpp]
    • a function to generate random string arrays/vector/boundedvector in c++
    • msg files and tests for all String/StringArray types
    • tests for PrimitivesStaticArrays that were not tested before
    • a disabled test for testing bounded string assignment (we currently don't have any way to restrict std::string length)
  • [rosidl_generator_c]
    • a TODO to remind us to test bounded strings once length restriction is enforced

@mikaelarguedas mikaelarguedas added the in review Waiting for review (Kanban column) label Dec 4, 2016
@mikaelarguedas mikaelarguedas self-assigned this Dec 4, 2016
{
std::default_random_engine rand_generator;
std::uniform_int_distribution<int> randnum(min, max);
std::uniform_int_distribution<int> randlen(minlength, maxlength);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend not to use random numbers for any tests. The problem is that the test will be flaky of the result depends on the values. Instead use a "known" pattern, e.g. increment numbers, or ascii characters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll ticket it and change it to use known patterns in a follow-up to keep the scope of this PR narrowed to string testing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ticketed here #181

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for not using fuzz values. I'm ok with doing it in a follow up if that's what you want.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

{
std::default_random_engine rand_generator;
std::uniform_int_distribution<int> randnum(min, max);
std::uniform_int_distribution<int> randlen(minlength, maxlength);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for not using fuzz values. I'm ok with doing it in a follow up if that's what you want.

@mikaelarguedas
Copy link
Member Author

Yeah I'll open a follow-up not to add noise to thise one.
Thanks for reviewing

@mikaelarguedas mikaelarguedas merged commit ed21607 into master Dec 8, 2016
@mikaelarguedas mikaelarguedas deleted the string_tests branch December 8, 2016 02:37
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants