colcon / colcon-cmake

Extension for colcon to support CMake packages
http://colcon.readthedocs.io
Apache License 2.0
16 stars 25 forks source link

Improve help message for --ctest-args #70

Closed rotu closed 4 years ago

rotu commented 4 years ago

colcon test --packages-select rosbag2_transport --ctest-args " --help" is expected to show help for ctest arguments but does not. Output is similar to if tests were run and succeeded.

colcon test --packages-select rosbag2_transport --ctest-args " --help"
Starting >>> rosbag2_transport
Finished <<< rosbag2_transport [0.19s]          

Summary: 1 package finished [1.06s]

The output from ctest --help can be found in log/latest_test/rosbag2_transport/stdout.log.

Perhaps the help text should come out in stdout or the help for the --ctest-args option should use an argument other than " --help" as in:

$ colcon test --help
usage: colcon test [-h] [--build-base BUILD_BASE] [--install-base INSTALL_BASE] [--merge-install]
...
Arguments for 'cmake' packages:
  --ctest-args [* [* ...]]
                        Pass arguments to CTest projects. Arguments matching other options must be prefixed by a space,
                        e.g. --ctest-args " --help"
dirk-thomas commented 4 years ago

Since colcon doesn't show stdout output by default and ctest --help prints to stdout (not stderr) this is expected behavior. You can pass e.g. --event-handlers console_direct+ to see the stdout output of the package.

rotu commented 4 years ago

I understand why it happens. Perhaps, since the —help option has such caveats, it would be better to use a different example in the help text.

dirk-thomas commented 4 years ago

... use a different example in the help text.

Please include the relevant context in you original ticket. It is very difficult to follow what you are referring to if you don't. In this case I assume you are referring to the help message of the --ctest-args option: https://github.com/colcon/colcon-cmake/blob/fff16c206886a63517c848792d3fb68601c4a8cd/colcon_cmake/task/cmake/build.py#L39-L41

rotu commented 4 years ago

Yep. I wrote this ticket in a pretty scattered way, but I think you caught the gist. If I were to try to lay out my thought process, here it is:

  1. I’d like to see how to use colcon effectively for running a single test case. In my case, I knew the test was kicked off by ctest. So I use colcon test —help to see what options might help me.
  2. The relevant argument for test selection is —ctest-args so let’s try the example it gives.
  3. Oops that didn’t seem to do anything...

My hangups here:

  1. No obvious way to choose the test case. User error? Discoverability problem? Who I think it should be fairly obvious how to run just a certain test case, though I still don’t know how.
  2. What are the valid options to colcon test —ctest-args? It might make sense to mention ctest —help in the help text here. And similar for other tools.
  3. In the arguments to be passed on to a sub-tool (Ctest), it’s a little weird to require space in a quoted arg if the expectation is that args are separated and not lexed out from a single multi-arg string. Maybe they should be a single string containing whitespace, maybe multiple arguments, but the hybrid seems ... weird.
  4. The actual example from the command line is strange. If you pass —ctest-args “ —help”, you get no help at the command line, whether help is run no times, once, or many times depends on present packages, and the actual help is tucked away in a log file that the user may not know exists.
  5. If you use the example from the command line, no tests are run and there is no warning about this.

I could split this into many issues, but it seems like it’s easier to have one “big picture” ticket about the ergonomics and discoverability of colcon/ctest.

dirk-thomas commented 4 years ago

I’d like to see how to use colcon effectively for running a single test case.

No obvious way to choose the test case. User error? Discoverability problem? Who I think it should be fairly obvious how to run just a certain test case, though I still don’t know how.

See https://colcon.readthedocs.io/en/released/user/how-to.html#run-specific-tests

What are the valid options to colcon test —ctest-args?

I assumed that it is obvious that valid options for --ctest-args are all the option ctest accepts. The help text also explicitly mentioned that these arguments are passed to CTest: https://github.com/colcon/colcon-cmake/blob/fff16c206886a63517c848792d3fb68601c4a8cd/colcon_cmake/task/cmake/test.py#L30

it’s a little weird to require space in a quoted arg if the expectation is that args are separated and not lexed out from a single multi-arg string.

This is a limitation of argparse and without implementing custom parsing logic (which I don't think should be done) the leading space is the recommended approach to work around that limitation.

Also passing a single quoted string containing all arguments isn't an option since you wouldn't be able to pass actual arguments which need to contain a space itself.

The actual example from the command line is strange. If you pass --ctest-args " --help", you get no help at the command line,

Agreed, the help string should be changed to something else.

If you use the example from the command line, no tests are run and there is no warning about this.

Well this is exactly what the user asked for by passing --help. Any existing tool like ctest or pytest will behave exactly the same - if you pass --help it doesn't actually run any tests.

I could split this into many issues, but it seems like it’s easier to have one “big picture” ticket about the ergonomics and discoverability of colcon/ctest.

A meta ticket like this has no clear acceptance criteria. Therefore it is hard if not impossible to act upon. Therefore please do not use this approach. Create separate tickets for separate items (using cross references to indicate the relation / context). That allows to iterated on each of them individually and close them when the specific part has been resolved / answered / etc.

For this ticket the acceptance criteria will be the not helpful help message of --ctest-args as the updated title indicates.

rotu commented 4 years ago

I agree with almost everything here.

The acceptance criteria is that "this ticket is fixed when passing arguments to --ctest-args is more discoverable and less confusing", and yes, it's very subjective. I trust your judgement.

dirk-thomas commented 4 years ago

For this ticket the acceptance criteria will be the not helpful help message of --ctest-args as the updated title indicates.

After merging the three referenced pull requests I consider this ticket resolved and plan to close it.

rotu commented 4 years ago

After merging the three referenced pull requests I consider this ticket resolved and plan to close it.

Agreed. I think I've punished this dead horse enough.