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

Add tests to the CTest framework #1015

Merged
merged 11 commits into from Jun 28, 2017

Conversation

Projects
None yet
4 participants
@shikharbhardwaj
Contributor

shikharbhardwaj commented May 31, 2017

Adds all the test suites in mlpack to CTest.

Building tests

Same as before. make mlpack_test

Running tests

In the build directory

ctest -jN

Need to set environment variable CTEST_OUTPUT_ON_FAILURE to 1 for ctest to provide detailed output from the failed test case.

Adding new tests

On introducing a new test suite, we need to add it to the testing framework by modifying src/mlpack/test/CMakeLists.txt

  1. Add the source file to the list of source for mlpack_test.(Same as before)

  2. Add the following line to add the test to CTest, TestSuiteName is the new test suite

    add_test(NAME TestSuiteName COMMAND mlpack_test -t TestSuiteName WORKING_DIRECTORY   ${CMAKE_BINARY_DIR})
    

Measurements

Testing times are reduced. My measurements.

I tried to modify .travis.yml to make ctest fire for running the tests, but I am not too sure it would work.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq May 31, 2017

Member

It's just an idea, is it possible to parse tests/CMakeLists.txt and the listed *test.cpp files for BOOST_AUTO_TEST_SUITE and autogenerate the add_test(...) commands?

Member

zoq commented May 31, 2017

It's just an idea, is it possible to parse tests/CMakeLists.txt and the listed *test.cpp files for BOOST_AUTO_TEST_SUITE and autogenerate the add_test(...) commands?

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin May 31, 2017

Member

It should be possible---I would imagine you can use CMake's string() function to make a regex to grab all of the names.

Member

rcurtin commented May 31, 2017

It should be possible---I would imagine you can use CMake's string() function to make a regex to grab all of the names.

@zoq zoq requested review from MarcosPividori and removed request for MarcosPividori Jun 1, 2017

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 2, 2017

Contributor

One way I thought of implementing this idea was by pulling the output from mlpack_test --list_content into a CMake string using add_custom_command on the mlpack_test target to fire an execute_process, removing all lines starting with a whitespace, splitting the resulting string on newlines using string(REGEX MATCHALL ...) and using the resulting list to add the tests. Is there any other way to achieve this?

I'll post an update soon. (The build broke on my system due to some upgrades I did)

Contributor

shikharbhardwaj commented Jun 2, 2017

One way I thought of implementing this idea was by pulling the output from mlpack_test --list_content into a CMake string using add_custom_command on the mlpack_test target to fire an execute_process, removing all lines starting with a whitespace, splitting the resulting string on newlines using string(REGEX MATCHALL ...) and using the resulting list to add the tests. Is there any other way to achieve this?

I'll post an update soon. (The build broke on my system due to some upgrades I did)

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jun 2, 2017

Member

That seems like a good approach to me. I can't think of a better way to do it off the top of my head, unless you wanted to procedurally open each _test.cpp file and extract the BOOST_AUTO_TEST_SUITE() name, but in my opinion it's easier to just run the mlpack_test --list_content command and work with the output from that.

Member

rcurtin commented Jun 2, 2017

That seems like a good approach to me. I can't think of a better way to do it off the top of my head, unless you wanted to procedurally open each _test.cpp file and extract the BOOST_AUTO_TEST_SUITE() name, but in my opinion it's easier to just run the mlpack_test --list_content command and work with the output from that.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 4, 2017

Contributor

I was able to extract the names of the test suites by doing the following.

In src/mlpack/test/CMakeLists.txt :

configure_file(generate_test_names.cmake.in generate_test_names.cmake @ONLY)
add_custom_command(TARGET mlpack_test
  POST_BUILD
  COMMAND ${CMAKE_COMMAND} -DPROJECT_BINARY_DIR=${PROJECT_BINARY_DIR} -P generate_test_names.cmake
)

In src/mlpack/test/generate_test_names.cmake.in :

function(get_names)
  message("Adding tests to the test suite")
  execute_process(COMMAND ${PROJECT_BINARY_DIR}/bin/mlpack_test --list_content
    OUTPUT_VARIABLE TEST_STRING)
  message(STATUS "TEST_STRING='${TEST_STRING}'")
  string(REGEX MATCHALL "^[^/ ]+[A-Za-z]" TEST_LIST ${TEST_STRING})
  # TEST_LIST has the list of all test suites
endfunction()

get_names()

But when I tried to add the tests within the script, CMake errored with Command add_test() is not scriptable. Any other ways we could automate this process?

Contributor

shikharbhardwaj commented Jun 4, 2017

I was able to extract the names of the test suites by doing the following.

In src/mlpack/test/CMakeLists.txt :

configure_file(generate_test_names.cmake.in generate_test_names.cmake @ONLY)
add_custom_command(TARGET mlpack_test
  POST_BUILD
  COMMAND ${CMAKE_COMMAND} -DPROJECT_BINARY_DIR=${PROJECT_BINARY_DIR} -P generate_test_names.cmake
)

In src/mlpack/test/generate_test_names.cmake.in :

function(get_names)
  message("Adding tests to the test suite")
  execute_process(COMMAND ${PROJECT_BINARY_DIR}/bin/mlpack_test --list_content
    OUTPUT_VARIABLE TEST_STRING)
  message(STATUS "TEST_STRING='${TEST_STRING}'")
  string(REGEX MATCHALL "^[^/ ]+[A-Za-z]" TEST_LIST ${TEST_STRING})
  # TEST_LIST has the list of all test suites
endfunction()

get_names()

But when I tried to add the tests within the script, CMake errored with Command add_test() is not scriptable. Any other ways we could automate this process?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 6, 2017

Member

Sounds like it's not allowed to do add_test(...) in script mode, not sure why. So I guess this brings us back to the idea to parse for the test suite names.

Member

zoq commented Jun 6, 2017

Sounds like it's not allowed to do add_test(...) in script mode, not sure why. So I guess this brings us back to the idea to parse for the test suite names.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 6, 2017

Contributor

Sure. I am working on using FILE(GLOB ..) to get all the test file sources and extracting the line with BOOST_AUTO_TEST_SUITE(...). Is it reasonable to assume that all test suites will be in separate files, each having a .cpp extension?

Edit : In the documentation of FILE,

We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.

I don't think this should be a problem, as when any new test is added, CMakeLists.txt is modified, forcing CMake to regenerate. Only if the test suite name is changed, this could cause an issue with the updated name not getting added to CTest without forcing regenration.

Contributor

shikharbhardwaj commented Jun 6, 2017

Sure. I am working on using FILE(GLOB ..) to get all the test file sources and extracting the line with BOOST_AUTO_TEST_SUITE(...). Is it reasonable to assume that all test suites will be in separate files, each having a .cpp extension?

Edit : In the documentation of FILE,

We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.

I don't think this should be a problem, as when any new test is added, CMakeLists.txt is modified, forcing CMake to regenerate. Only if the test suite name is changed, this could cause an issue with the updated name not getting added to CTest without forcing regenration.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 6, 2017

Member

I guess we could make the assumption that the file ends with .cpp on the other side I'm not sure we can assume that each file in tests/ that also contains BOOST_AUTO_TEST_SUITE(...) should be added to the test suite. There might be a situation where someone likes to exclude test files, but I think it would be save to parse: https://github.com/mlpack/mlpack/blob/master/src/mlpack/tests/CMakeLists.txt for all cpp files and go from there.

Member

zoq commented Jun 6, 2017

I guess we could make the assumption that the file ends with .cpp on the other side I'm not sure we can assume that each file in tests/ that also contains BOOST_AUTO_TEST_SUITE(...) should be added to the test suite. There might be a situation where someone likes to exclude test files, but I think it would be save to parse: https://github.com/mlpack/mlpack/blob/master/src/mlpack/tests/CMakeLists.txt for all cpp files and go from there.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 6, 2017

Contributor

Instead of using GLOB, I directly picked the sources from the target mlpack_test. A little change to the parsing for taking up both BOOST_AUTO_TEST_SUITE and BOOST_FIXTURE_TEST_SUITE.

Should I remove the listing of test names, to keep the logs clean?

Contributor

shikharbhardwaj commented Jun 6, 2017

Instead of using GLOB, I directly picked the sources from the target mlpack_test. A little change to the parsing for taking up both BOOST_AUTO_TEST_SUITE and BOOST_FIXTURE_TEST_SUITE.

Should I remove the listing of test names, to keep the logs clean?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 6, 2017

Member

That would be great, reduce the output to a minimum.

Member

zoq commented Jun 6, 2017

That would be great, reduce the output to a minimum.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jun 6, 2017

Member

Looks nice; I'm not sure why it isn't giving a speedup to the Travis build time though (seems to still be ~36 minutes). Do you get speedup when you run it locally?

One other question---how can we get the XML reports out of the boost unit test framework for Jenkins builds? I'm not sure if CTest has support for that; maybe we will have to run the tests manually?

Member

rcurtin commented Jun 6, 2017

Looks nice; I'm not sure why it isn't giving a speedup to the Travis build time though (seems to still be ~36 minutes). Do you get speedup when you run it locally?

One other question---how can we get the XML reports out of the boost unit test framework for Jenkins builds? I'm not sure if CTest has support for that; maybe we will have to run the tests manually?

Show outdated Hide outdated src/mlpack/tests/CMakeLists.txt
Show outdated Hide outdated src/mlpack/tests/CMakeLists.txt
Show outdated Hide outdated src/mlpack/tests/CMakeLists.txt
@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 7, 2017

Contributor

I did get a speed up when I ran the tests locally (with -j 4). Here are the timings. On Travis, its set to run at -j 2. I believe this is because some of the tests are multi threaded, and with less threads available at hand, the sequential tests do not get much more CPU time to run in parallel with each other.

I am not too familiar with Jenkins, but CTest does generate XML files for the tests executed (with the -T Test) option, under the Testing folder. I found Jenkins has added support for this format here : https://issues.jenkins-ci.org/browse/JENKINS-17884.

Contributor

shikharbhardwaj commented Jun 7, 2017

I did get a speed up when I ran the tests locally (with -j 4). Here are the timings. On Travis, its set to run at -j 2. I believe this is because some of the tests are multi threaded, and with less threads available at hand, the sequential tests do not get much more CPU time to run in parallel with each other.

I am not too familiar with Jenkins, but CTest does generate XML files for the tests executed (with the -T Test) option, under the Testing folder. I found Jenkins has added support for this format here : https://issues.jenkins-ci.org/browse/JENKINS-17884.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 7, 2017

Member

We could test if using -j 4 for the Travis build has any positive effect.

Member

zoq commented Jun 7, 2017

We could test if using -j 4 for the Travis build has any positive effect.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 7, 2017

Contributor

Travis allocates only 2 cores to the build. But we could try and see if that reduces the timings (although highly unlikely).
Regarding Jenkins support, does boost unit test support test suite timings? Every test here : http://masterblaster.mlpack.org/job/mlpack%20-%20git%20commit%20test/lastCompletedBuild/testReport/mlpackTest/ has the same duration.

Contributor

shikharbhardwaj commented Jun 7, 2017

Travis allocates only 2 cores to the build. But we could try and see if that reduces the timings (although highly unlikely).
Regarding Jenkins support, does boost unit test support test suite timings? Every test here : http://masterblaster.mlpack.org/job/mlpack%20-%20git%20commit%20test/lastCompletedBuild/testReport/mlpackTest/ has the same duration.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 7, 2017

Member

Sometimes the plugin merges the timings, not sure why, but if you open the TestSuite you see the correct timings.

Member

zoq commented Jun 7, 2017

Sometimes the plugin merges the timings, not sure why, but if you open the TestSuite you see the correct timings.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 7, 2017

Contributor

Ah that's right. I can see timings of the individual test cases.

If we integrate CTest with Jenkins, I don't think we'll get individual testcase timings. Here is a CTest generated XML file for the tests : https://gist.github.com/shikharbhardwaj/e09f0aba257c366ca84f7b86c2d59305.

CTest gives out the timings and status on the test suite level, not on the test case level.
The existing Jenkins configuration would still work with the above changes, as the test executable is still built in the same manner.

Contributor

shikharbhardwaj commented Jun 7, 2017

Ah that's right. I can see timings of the individual test cases.

If we integrate CTest with Jenkins, I don't think we'll get individual testcase timings. Here is a CTest generated XML file for the tests : https://gist.github.com/shikharbhardwaj/e09f0aba257c366ca84f7b86c2d59305.

CTest gives out the timings and status on the test suite level, not on the test case level.
The existing Jenkins configuration would still work with the above changes, as the test executable is still built in the same manner.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jun 7, 2017

Member

I suspect it might be useful, on machines with OpenBLAS, to specify OMP_NUM_THREADS=1 before each test run, otherwise e.g. on an 8-core system you'd have 8 tests all running in parallel trying to use 8 threads.

I think for Jenkins we should just keep running the tests manually so that we can pass options to the Boost Unit Test Framework to get the XML output in the format we need. But definitely the results you are seeing show a significant test time improvement and I'll start using ctest immediately for my own personal testing after we merge this. :)

Member

rcurtin commented Jun 7, 2017

I suspect it might be useful, on machines with OpenBLAS, to specify OMP_NUM_THREADS=1 before each test run, otherwise e.g. on an 8-core system you'd have 8 tests all running in parallel trying to use 8 threads.

I think for Jenkins we should just keep running the tests manually so that we can pass options to the Boost Unit Test Framework to get the XML output in the format we need. But definitely the results you are seeing show a significant test time improvement and I'll start using ctest immediately for my own personal testing after we merge this. :)

@mentekid

This comment has been minimized.

Show comment
Hide comment
@mentekid

mentekid Jun 7, 2017

Contributor

Sorry for not contributing to this sooner, I am just rubbish when it comes to CMAKE.

I tested the code locally and it looks good, just like your results.

My only concern about this is that if ctest uses OpenMP to parallelize the test execution, that might cause our own parallel code to run sequentially (by default, nested multithreading is off). This is OK in general, but not for the parallel tests. However, I am not sure this is going to be a problem. I am testing this now locally and will report back if I see anything weird.

Contributor

mentekid commented Jun 7, 2017

Sorry for not contributing to this sooner, I am just rubbish when it comes to CMAKE.

I tested the code locally and it looks good, just like your results.

My only concern about this is that if ctest uses OpenMP to parallelize the test execution, that might cause our own parallel code to run sequentially (by default, nested multithreading is off). This is OK in general, but not for the parallel tests. However, I am not sure this is going to be a problem. I am testing this now locally and will report back if I see anything weird.

@mentekid

This comment has been minimized.

Show comment
Hide comment
@mentekid

mentekid Jun 10, 2017

Contributor

I've tested my concern a bit, and it looks like the LSH tests that are supposed to run in parallel actually do. Shikhar also told me that the -j flag actually spawns processes, and not threads, so I guess there is nothing to worry about.

The Travis failure looks random, so if everyone's happy I say LGTM :)

Contributor

mentekid commented Jun 10, 2017

I've tested my concern a bit, and it looks like the LSH tests that are supposed to run in parallel actually do. Shikhar also told me that the -j flag actually spawns processes, and not threads, so I guess there is nothing to worry about.

The Travis failure looks random, so if everyone's happy I say LGTM :)

@zoq

zoq approved these changes Jun 12, 2017

I agree, if you could take care of the issue I pointed out above, I think this is ready to be merged.

Show outdated Hide outdated src/mlpack/tests/CMakeLists.txt
@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 12, 2017

Contributor

I have added a few general comments around the additional code. I'll add more specific comments if needed.

Contributor

shikharbhardwaj commented Jun 12, 2017

I have added a few general comments around the additional code. I'll add more specific comments if needed.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jun 13, 2017

Member

My only concern about this is that if ctest uses OpenMP to parallelize the test execution, that might cause our own parallel code to run sequentially (by default, nested multithreading is off). This is OK in general, but not for the parallel tests. However, I am not sure this is going to be a problem. I am testing this now locally and will report back if I see anything weird.

Hmm, I don't think that ctest uses OpenMP, but wouldn't we want each test to run in a singlethreaded mode if ctest is running tests in parallel? That way each test has its own core, instead of e.g. on an 8-core system ctest would spawn 8 tests that each tried to use 8 cores. Let me know if I misunderstood something.

Member

rcurtin commented Jun 13, 2017

My only concern about this is that if ctest uses OpenMP to parallelize the test execution, that might cause our own parallel code to run sequentially (by default, nested multithreading is off). This is OK in general, but not for the parallel tests. However, I am not sure this is going to be a problem. I am testing this now locally and will report back if I see anything weird.

Hmm, I don't think that ctest uses OpenMP, but wouldn't we want each test to run in a singlethreaded mode if ctest is running tests in parallel? That way each test has its own core, instead of e.g. on an 8-core system ctest would spawn 8 tests that each tried to use 8 cores. Let me know if I misunderstood something.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 13, 2017

Contributor

In the current way of running tests, the maximum number of threads used during testing could go as high as N*OMP_NUM_THREADS (N is the number of test jobs), when N parallelized test jobs are started at once. But by measuring, I saw that running the parallelized tests separately was slower than just queuing them to the OS scheduler and letting it manage the CPU time allocated to each test. This is because some of the parallel tests do not use 100% CPU time of each thread they allocate.

In the worst case, this strategy can cause a slowdown when the random test scheduling takes up multiple parallel tests at once. Its a tradeoff between better average case performance and better worst case performance.

This may also be concerning in situations when we don't want the tests to use more than X physical threads at a time. If this is an important issue, then the best approach would be to maintain a list of long running(> 10s) parallelized tests and use RUN_SERIAL with them, so the maximum threads used during testing would not exceed max(N, OMP_NUM_THREADS).

Reducing OMP_NUM_THREADS would be an option, but then the tests which require more CPU time would run longer(with lesser threads to work with).

I'll push the changes with RUN_SERIAL to see if they perform better on Travis.

Contributor

shikharbhardwaj commented Jun 13, 2017

In the current way of running tests, the maximum number of threads used during testing could go as high as N*OMP_NUM_THREADS (N is the number of test jobs), when N parallelized test jobs are started at once. But by measuring, I saw that running the parallelized tests separately was slower than just queuing them to the OS scheduler and letting it manage the CPU time allocated to each test. This is because some of the parallel tests do not use 100% CPU time of each thread they allocate.

In the worst case, this strategy can cause a slowdown when the random test scheduling takes up multiple parallel tests at once. Its a tradeoff between better average case performance and better worst case performance.

This may also be concerning in situations when we don't want the tests to use more than X physical threads at a time. If this is an important issue, then the best approach would be to maintain a list of long running(> 10s) parallelized tests and use RUN_SERIAL with them, so the maximum threads used during testing would not exceed max(N, OMP_NUM_THREADS).

Reducing OMP_NUM_THREADS would be an option, but then the tests which require more CPU time would run longer(with lesser threads to work with).

I'll push the changes with RUN_SERIAL to see if they perform better on Travis.

@shikharbhardwaj

This comment has been minimized.

Show comment
Hide comment
@shikharbhardwaj

shikharbhardwaj Jun 13, 2017

Contributor

As a side note, I have noticed KRANNTest is failing in every run under CTest on Travis. Any ideas what could be causing this? Is there some kind of dependency in the tests?

Contributor

shikharbhardwaj commented Jun 13, 2017

As a side note, I have noticed KRANNTest is failing in every run under CTest on Travis. Any ideas what could be causing this? Is there some kind of dependency in the tests?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jun 13, 2017

Member

It's a know issue that the RANNTest/RAModelTest sometimes fails; for more information take a look at: #922

Member

zoq commented Jun 13, 2017

It's a know issue that the RANNTest/RAModelTest sometimes fails; for more information take a look at: #922

@rcurtin

rcurtin approved these changes Jun 16, 2017 edited

I'm a little concerned that parallel_tests might be a bit hard to maintain, but I think it is the best solution for now. As mlpack becomes more and more parallel, I guess more of the tests will run serially. I also hadn't realized how little of the Travis build time goes to testing---the vast majority is just compilation! :)

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jun 27, 2017

Member

Do we all think this one is ready to merge? I think everyone has approved it by now. @mentekid?

Member

rcurtin commented Jun 27, 2017

Do we all think this one is ready to merge? I think everyone has approved it by now. @mentekid?

@mentekid

This comment has been minimized.

Show comment
Hide comment
@mentekid

mentekid Jun 28, 2017

Contributor

Sorry, I missed this one, I thought it was already merged. I'll merge it yeah.

Contributor

mentekid commented Jun 28, 2017

Sorry, I missed this one, I thought it was already merged. I'll merge it yeah.

@mentekid mentekid merged commit 0a0c51d into mlpack:master Jun 28, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment