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

Increase MAX_STRING_SIZE #487

Merged
merged 2 commits into from
Aug 23, 2019
Merged

Increase MAX_STRING_SIZE #487

merged 2 commits into from
Aug 23, 2019

Conversation

hyunseok-yang
Copy link
Contributor

It's too short for string length.
It occurs the error when the string field in yaml files are too long....

It's too short for string length.
It occurs the error when the string field in yaml files are too long....

Signed-off-by: Hyunseok Yang <hyunseok7.yang@lge.com>
@dirk-thomas
Copy link
Member

The branch name suggests you created the patch using the GitHub web interface. Before creating a PR please make sure that the proposed change builds and passes all tests successfully.

In this case the test checking for the possible maximum size (https://github.com/ros2/rcl/blob/b66395dc570b5c270dd0da4a1b0b121e5617a26e/rcl_yaml_param_parser/test/max_string_sz.yaml) does fail at the moment and need to be updated accordingly.

@dirk-thomas dirk-thomas added in progress Actively being worked on (Kanban column) requires-changes labels Aug 21, 2019
@hyunseok-yang
Copy link
Contributor Author

hyunseok-yang commented Aug 21, 2019

The branch name suggests you created the patch using the GitHub web interface. Before creating a PR please make sure that the proposed change builds and passes all tests successfully.

In this case the test checking for the possible maximum size (https://github.com/ros2/rcl/blob/b66395dc570b5c270dd0da4a1b0b121e5617a26e/rcl_yaml_param_parser/test/max_string_sz.yaml) does fail at the moment and need to be updated accordingly.

I think i got your point.
Unfortunately.. I don't know how to run the all tests exactly through colcon(?).
(@dirk-thomas could you let me know if there is some guidance for running tests)

But, I've tried like these.
(I'm not sure if it is correct)

  1. Go to the 'rcl_yaml_param_parser' directory
  2. make the "build" directory
  3. run "cmake ../"
  4. run "make test"

Then the result was like below.

Anyone knows what's the problem... here?

Running tests...
Test project /home/yg/Work/ros2_work/src/rcl/rcl_yaml_param_parser/build
    Start 1: test_parse_yaml
1/7 Test #1: test_parse_yaml ..................***Failed    0.05 sec
    Start 2: copyright
2/7 Test #2: copyright ........................***Failed    0.19 sec
    Start 3: cppcheck
3/7 Test #3: cppcheck .........................   Passed    0.38 sec
    Start 4: cpplint
4/7 Test #4: cpplint ..........................***Failed    0.61 sec
    Start 5: lint_cmake
5/7 Test #5: lint_cmake .......................***Failed    0.20 sec
    Start 6: uncrustify
6/7 Test #6: uncrustify .......................***Failed    0.28 sec
    Start 7: xmllint
7/7 Test #7: xmllint ..........................   Passed    0.58 sec

29% tests passed, 5 tests failed out of 7

Label Time Summary:
copyright     =   0.19 sec*proc (1 test)
cppcheck      =   0.38 sec*proc (1 test)
cpplint       =   0.61 sec*proc (1 test)
gtest         =   0.05 sec*proc (1 test)
lint_cmake    =   0.20 sec*proc (1 test)
linter        =   2.23 sec*proc (6 tests)
uncrustify    =   0.28 sec*proc (1 test)
xmllint       =   0.58 sec*proc (1 test)

Total Test time (real) =   2.28 sec

The following tests FAILED:
	  1 - test_parse_yaml (Failed)
	  2 - copyright (Failed)
	  4 - cpplint (Failed)
	  5 - lint_cmake (Failed)
	  6 - uncrustify (Failed)
Errors while running CTest
Makefile:107: recipe for target 'test' failed
make: *** [test] Error 8

@dirk-thomas
Copy link
Member

I don't know how to run the all tests exactly through colcon.

As described in the docs there is a test verb for colcon similar to build. In this case you can further reduce the tests to the changed package:

colcon test --packages-select rcl_yaml_param_parser

@hyunseok-yang
Copy link
Contributor Author

I don't know how to run the all tests exactly through colcon.

As described in the docs there is a test verb for colcon similar to build. In this case you can further reduce the tests to the changed package:

colcon test --packages-select rcl_yaml_param_parser

really appreciate it for guidance! 👍

BTW, I tried the test as you explained...

It seems gtest was all passed.

1: [ RUN      ] test_file_parser.maximum_number_parameters
1: cur_path: /home/yg/Work/ros2_work/src/rcl/rcl_yaml_param_parser/test/max_num_params.yaml
1: Exceeded maximum allowed number of parameters for a node (512), at /home/yg/Work/ros2_work/src/rcl/rcl_yaml_param_parser/src/parser.c:1145
1: [       OK ] test_file_parser.maximum_number_parameters (1 ms)
1: [----------] 13 tests from test_file_parser (3 ms total)
1: 
1: [----------] Global test environment tear-down
1: [==========] 13 tests from 1 test case ran. (3 ms total)
1: [  PASSED  ] 13 tests.

But following test was failed... and I think those are the problems.

Total Test time (real) =   1.20 sec

The following tests FAILED:
	  2 - copyright (Failed)
	  3 - cppcheck (Failed)
	  4 - cpplint (Failed)
	  5 - lint_cmake (Failed)
	  6 - uncrustify (Failed)
	  7 - xmllint (Failed)

Am I missing any other settings...?
I can not follow the reason why cppcheck, cpplint, xmllint or copyright is occurring errors..
I just only changed the define value. :(

I will try several things during wait your reply.

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
Copy link
Member

I don't know why it passes for you. I updated the test to match the new limit: ef6da05 With this change the PR build passes again.

Thanks for the update.

For all the linter tests I would suggest to double check that you went through all the steps of the install instructions. If you want to dig into why they are failing you could also look into each result file listed by colcon test-result which will contain more information.

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) requires-changes labels Aug 23, 2019
@dirk-thomas dirk-thomas merged commit d070038 into ros2:master Aug 23, 2019
dirk-thomas pushed a commit that referenced this pull request Sep 19, 2019
* Increase MAX_STRING_SIZE

It's too short for string length.
It occurs the error when the string field in yaml files are too long....

Signed-off-by: Hyunseok Yang <hyunseok7.yang@lge.com>

* update test to match increased limit

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants