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.

Sign up for GitHub

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

Jump to bottom

configurable run test verbosity #267

Merged
merged 3 commits into from
Feb 2, 2018
Merged

configurable run test verbosity #267

merged 3 commits into from
Feb 2, 2018

Conversation

fmessmer
Copy link
Contributor

@fmessmer fmessmer commented Jan 16, 2018 •
edited
Loading

follow-up of #255 (comment)

@ipa-mdl
I'm not sure how the environment variable handling works in industrial_ci
also, I was not sure whether/how to set VERBOSE_TESTS to true by default

please guide me here - or take over the branch 😉

@fmessmer fmessmer force-pushed the configurable_run_test_verbosity branch from b5e0852 to cbd7143 Compare January 16, 2018 09:26
@fmessmer
Copy link
Contributor Author

this feature reduces our log length from >10000 lines down to 2500!

mathias-luedtke
mathias-luedtke requested changes Jan 17, 2018
View reviewed changes
industrial_ci/src/tests/source_tests.sh Outdated
@@ -25,11 +25,18 @@
ici_require_run_in_docker # this script must be run in docker

#Define some verbose env vars
#verbose build tests
Copy link
Member

Choose a reason for hiding this comment

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

It's just "verbose build", not just tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mathias-luedtke
Copy link
Member

Just some minor glitch, the rest LGTM

@fmessmer fmessmer force-pushed the configurable_run_test_verbosity branch from cbd7143 to 8d0f665 Compare January 18, 2018 08:28
fmessmer
fmessmer commented Jan 18, 2018
View reviewed changes
industrial_ci/src/tests/source_tests.sh Outdated
@@ -187,7 +194,7 @@ if [ "$NOT_TEST_BUILD" != "true" ]; then

ici_time_start catkin_run_tests
if [ "$BUILDER" == catkin ]; then
catkin run_tests --no-deps --no-status $PKGS_DOWNSTREAM $CATKIN_PARALLEL_TEST_JOBS --make-args $ROS_PARALLEL_TEST_JOBS --
catkin build --no-deps --catkin-make-args run_tests -- $OPT_RUN_VI --no-status $PKGS_DOWNSTREAM $CATKIN_PARALLEL_TEST_JOBS --make-args $ROS_PARALLEL_TEST_JOBS --
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I add --summarize here too - for the sake of consistency with the other build steps?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure, if it add something interesting.
You can just have a look and decide yourself.

@mathias-luedtke
Copy link
Member

Just had a look at this https://travis-ci.org/ros-industrial/industrial_ci/jobs/330252621
(compared to https://travis-ci.org/ros-controls/ros_control/jobs/327897154)
The output verbosity was reduced dramatically, but now the ROS_ERROR line don't have an context.

I think OPT_RUN_VI="-vi" needs to be changed to OPT_RUN_VI="-v" for verbose runs.
And perhaps we should make it the default.
@130s: What do you think?

@fmessmer
Copy link
Contributor Author

fmessmer commented Feb 1, 2018

@ipa-mdl @130s
in bea3aa3, I removed the interleave-output
Let me know what you like better

For making it default, I would need a pointer to an example showing how this is done!

mathias-luedtke
mathias-luedtke reviewed Feb 1, 2018
View reviewed changes
industrial_ci/src/tests/source_tests.sh Outdated
OPT_RUN_V="-v"
else
OPT_RUN_V=""
fi

Copy link
Member

Choose a reason for hiding this comment

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

if [ "$VERBOSE_TESTS" = false ]; then
    OPT_RUN_V=""
else
    OPT_RUN_V="-v"
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😉 I thought it'd be more complicated ^^
(but I'll wait until the decision with/without interleave is made)

@mathias-luedtke
Copy link
Member

in bea3aa3, I removed the interleave-output
Let me know what you like better

Yes, his was the default before and definately makes sense für the tests.
In interleaved mode you might see a lot of error line of tests that check if they occur without any chance to link them to specific tests cases.

@fmessmer
Copy link
Contributor Author

fmessmer commented Feb 1, 2018

@ipa-mdl @130s
happy?

@mathias-luedtke
Copy link
Member

I am happy :)

@130s: Should tests be verbose by default or not?

@fmessmer fmessmer force-pushed the configurable_run_test_verbosity branch from 06b529f to 75e37f6 Compare February 2, 2018 08:30
@fmessmer
Copy link
Contributor Author

fmessmer commented Feb 2, 2018

@130s @ipa-mdl
although, I can't see @130s comments here in the issue, I got them per git notification
thus, I considered them and re-ordered/squashed the commits
now everything should be good to merge

130s
130s approved these changes Feb 2, 2018
View reviewed changes
Copy link
Member

@130s 130s left a comment

Choose a reason for hiding this comment

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

Thanks for the iteration. +1 for merge once Travis passes.

@mathias-luedtke mathias-luedtke merged commit 35d1f59 into ros-industrial:master Feb 2, 2018
@fmessmer fmessmer mentioned this pull request Feb 2, 2018
Closed
5 tasks
@fmessmer fmessmer deleted the configurable_run_test_verbosity branch March 5, 2021 08:42
@mathias-luedtke mathias-luedtke mentioned this pull request Jul 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathias-luedtke mathias-luedtke mathias-luedtke approved these changes

@130s 130s 130s approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fmessmer @mathias-luedtke @130s