search

colcon / colcon-cmake

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

Ensure dashboards updated after tests run #71

Closed rotu closed 4 years ago

rotu commented 4 years ago

Basic sanity test that CTest actually ran tests.

dirk-thomas commented 4 years ago

Please provide reproducible steps where this is a problem and ctest doesn't generate a result.

rotu commented 4 years ago

colcon test --packages-select osrf_testing_tools_cpp --ct /V

dirk-thomas commented 4 years ago

colcon test --packages-select osrf_testing_tools_cpp --ct /V

If I understand this correctly the user explicity asked to run ctest /V which implies not running any tests. Why should that result in a warning if it does exactly what the user requested?

rotu commented 4 years ago

Because the intent is to run tests and update the dashboard. If user flags can make that not happen, it violates the implied post-condition of the CmakeTestTask.test(...) function (i.e. production of a results file that contains test results). The user may have accidentally left an unwanted flag in there or misspelled another flag (-V). Or something could have affected the CTest config to put result files outside the build folder.

dirk-thomas commented 4 years ago

Because the intent is to run tests and update the dashboard. If user flags can make that not happen, it violates the implied post-condition of the CmakeTestTask.test(...) function (i.e. production of a results file that contains test results).

I have to disagree with you on this. The "post-condition" to generate test result is not unconditional.

To make a different example: if a user passes e.g. a --dry-run option to a tool he doesn't expect to see a warning that the run didn't do anything. The post-condition to do something is being overridden by the explicit option passed by the user.

The user may have accidentally left an unwanted flag in there or misspelled another flag (-V).

If the user passes a misspelled flag which doesn't exist I expect the invoked tool to return an error code. An example for this would be to pass --cmake-args --foobarbaz which will make cmake return an error code.

rotu commented 4 years ago

I have to disagree with you on this. The "post-condition" to generate test result is not unconditional.

If you don't think so, then you ought to close this PR. But the plugin IS NOT CTest and I think it's a sensible thing to warn about, especially when the default is to hide output from the user, and when the user might only be slightly familiar with CTest and/or the other testing tools that colcon test may invoke.

To make a different example: if a user passes e.g. a --dry-run option to a tool he doesn't expect to see a warning that the run didn't do anything. The post-condition to do something is being overridden by the explicit option passed by the user.

That's a reasonable expectation, but it's incorrect. If you pass colcon test --ctest-args " -N" (-N,--show-only[=format] = Disable actual execution of tests.), the tests still get run and the dashboard file still gets produced.

If the user passes a misspelled flag which doesn't exist I expect the invoked tool to return an error code.

Again, a reasonable expectation, and one that CTest does not follow. It silently ignores unparsed arguments.

dirk-thomas commented 4 years ago

But the plugin IS NOT CTest

I don't understand what you mean with this? Maybe you can clarify.

If you pass colcon test --ctest-args " -N" (-N,--show-only[=format] = Disable actual execution of tests.), the tests still get run and the dashboard file still gets produced.

I would be surprised by that. I just tried the command and it certainly does not run the tests.

It does generate the dashboard file though (stating that the test list is empty). That seems like an interesting choice of ctest but I don't see anything wrong with it. Arguable ctest could skip generating the dashboard file if -N is passed. You could consider to report that as an upstream ticket to CMake.

Again, a reasonable expectation, and one that CTest does not follow. It silently ignores unparsed arguments.

Unfortunate that this is how CTest behaves. I would recommend to fill an issue upstream. I don't think colcon should be responsible to workaround whatever undesired default behavior of used tools.

rotu commented 4 years ago

I don't understand what you mean with this? Maybe you can clarify.

I mean the plugin exists in service of the CTest command, whose functionality is "test a set of packages" not "run CTest on a set of packages". Sure the user can pass flags that make it not do that, but at the end of the day, that means tests were not run and a warning is apt. That --ctest-args=" help" can sometimes work seems just a convenient accident (if you run it on a package that happens to have the right tests and you fudge the output settings so you can see the help text).

colcon test --help
usage: colcon test [-h] [--build-base BUILD_BASE] [--install-base INSTALL_BASE] [--merge-install]
...

Test a set of packages.

I would be surprised by that. I just tried the command and it certainly does not run the tests.

Oops. You're right. I saw the logger output and that the dashboard file had changed and didn't look at the file contents.

I don't think colcon should be responsible to workaround whatever undesired default behavior of used tools.

:+1:

dirk-thomas commented 4 years ago

Thanks for clarifying. Well, I still think the tool should not warn when it does exactly what the user asked to do. Therefore I will go ahead and close this for now.

Please feel free to continue commenting. Also if others want to raise an opinion please feel free to do so and if necessary this can be reopened.