Teach TRIBITS_ADD_ADVANCED_TEST() new TEST_<IDX> block type COPY_FILES_TO_TEST_DIR

[bartlettroscoe]

Summary: Teach TRIBITS_ADD_ADVANCED_TEST() new TEST_<IDX> block type COPY_FILES_TO_TEST_DIR

CC: @fryeguy52

Description:

A very common idiom with the usage of TRIBITS_ADD_ADVANCED_TEST() is to copy files from the source tree to the test directory when a test runs. That has several advantages:

• One can modify the input files and then just run the test with ctest in an iterative manner (and not have to configure again with CONFIGURE_FILE( ... COPYONLY) or build again with TRIBITS_COPY_FILES_TO_BINARY_DIR() in order to copy files).
• The test directory gets blow away every time before it runs and therefore any old files are deleted before the test gets run again (which avoids the problem of having a test pass looking for the old files that will not be there when someone configures and builds from scratch).

Because of these advantages, projects like CASL VERA that use TriBITS almost always copy the files needed to run the test inside of the call to TRIBITS_ADD_ADVANCED_TEST() itself. That is not too hard to do using various commands like cmake -E copy <from> <to> but that is tedious to write the input blocks for a bunch of files to copy.

Therefore, this Story is to add a new type of TRIBITS_ADD_ADVANCED_TEST() TEST_<IDX> block called COPY_FILES_TO_TEST_DIR (in addition to CMND and EXEC blocks) to copy files in bulk, with the structure:

TRIBITS_ADD_ADVANCED_TEST(<testName>
  [...]
  TEST_<IDX> COPY_FILES_TO_TEST_DIR <file0> <file1> ...
     [SOURCE_DIR <srcBaseDir>]
     [DEST_DIR <destBaseDir>]
  [...]

If SOURCE_DIR <srcBaseDir> is omitted, then <srcBaseDir> is assumed to be ${CMAKE_CURRENT_SOURCE_DIR}. If SOURCE_DIR <srcBaseDir> is provided and <srcBaseDir> is a relative path, then it is assumed to be relative to ${CMAKE_CURRENT_SOURCE_DIR}. If <srcBaseDir> is an absolute path, then that path is used without question.

If DEST_DIR <destBaseDir> is omitted, then <destBaseDir> is assumed to be the current directory the binary where the test is runnin (usually ${CMAKE_CURRENT_BINARY_DIR}/<fullTestName>). If DEST_DIR <destBaseDir> is provided, and <destBaseDir> is a relative path, then it is assumed to be relative to the directory the test is running in. (If there is a subdir path given, then that directory structure will be created under the binary test directory). If <destBaseDir> is a absolute path, then that path is used without question. (However, it is not recommended to copy files into a directory that is not under the binary test directory as that removes some of the advantages of copying test files to the test directory inside of a test invocation in the first place).

While this looks to be a simple feature to add, it has to involve the following:

• Strong automated tests for every use case
• Strong acceptance tests that ensures that files are copied correctly.
• Full documentation for TRIBITS_ADD_ADVANCED_TEST().

Implementation Ideas:

The simplest way to implement this is likely to just implement the copy directly inside of the file DriveAdvancedTest.cmake using the command CONFIGURE_FILE(<sourceBaseDir>/<filei> <destBaseDir>/<filei> COPYONLY). That could print a summary like:

Copying files from <sourceBaseDir>/ to <destBaseDir>/:
  <file0> ...
  <file1> ...
  <file2> ...

And it would show the file copy lines in real time and if timing was turned on, it would show timing for copying all of the files in bulk.

The greatest amount of work will be testing this. There is also the chance of breaking backward compatibility. That is, if someone was already using COPY_FILES_TO_TEST_DIR inside of some TRIBITS_ADD_ADVANCED_TEST() call, then this will break that. But that seems very unlikely.

[bartlettroscoe]

We need this for the TriBITS SPARC prototype build system ASAP (ATDV-84) so I will get working on this right now.

@fryeguy52,

Can you please review the spec for this in the above "Description" field and see if you see anything off?

And once I have an initial implementation in a branch complete, can you please do a quick review on the PR branch?

[fryeguy52]

@bartlettroscoe,

Overall this looks good to me. One question, will you be able to "configure" files that you copy like in CONFIGURE_FILE(...)?

[bartlettroscoe]

Overall this looks good to me. One question, will you be able to "configure" files that you copy like in CONFIGURE_FILE(...)?

I don't think we can directly support that since CONFIGURE_FILE(...) runs in a separate cmake -P script that is invoked by ctest. If a user wants to configure a file (with substitutions), they would have to first copy/configure the file into the build directory (at configure time) and then copy it from the build dir to the test dir at test time using this new TEST_<IDX> command block . I can see some use cases where that might be useful. We could mention that as a possible variation/option that users could use.

Does that make sense?

[bartlettroscoe]

The full implementation is basically there now in the branch:

• https://github.com/TriBITSPub/TriBITS/tree/proto-tribits-system-atdv-84

As long as all of the files copy correctly, everything is fine (and I am already using this with the SPARC TriBITS prototype build system). The problem is what happens when a file does not copy (implemented with CONFIGURE_FILE(... COPYONLY)). The ctest -P <test_name>.cmake script just dies immediately with:

58: XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
58: 
58: Advanced Test: TriBITS_CTestScripts_TAAT_COPY_FILES_TO_TEST_DIR_bad_file_name
58: 
58: Selected Test/CTest Propeties:
58:   CATEGORIES = BASIC
58:   PROCESSORS = 1
58:   TIMEOUT    = DEFAULT
58: 
58: Creating new working directory '/home/rabartl/Trilinos.base2/BUILDS/GCC-4.8.3/TRIBITS_MPI_DEBUG/test/core/CTestScriptsUnitTests/TriBITS_CTestScripts_TAAT_COPY_FILES_TO_TEST_DIR_bad_file_name'
58: 
58: Running test commands: TEST_0, TEST_1
58: 
58: ================================================================================
58: 
58: TEST_0
58: 
58: Copy files from:
58:     '/ascldap/users/rabartl/Trilinos.base2/Trilinos/TriBITS/test/core/CTestScriptsUnitTests/'
58:   to:
58:     '/ascldap/users/rabartl/Trilinos.base2/BUILDS/GCC-4.8.3/TRIBITS_MPI_DEBUG/test/core/CTestScriptsUnitTests/TriBITS_CTestScripts_TAAT_COPY_FILES_TO_TEST_DIR_bad_file_name/' ...
58: 
58:   this_file_does_not_exist.txt ...
58: CMake Error: File /ascldap/users/rabartl/Trilinos.base2/Trilinos/TriBITS/test/core/CTestScriptsUnitTests/this_file_does_not_exist.txt does not exist.
58: CMake Error at /ascldap/users/rabartl/Trilinos.base2/Trilinos/TriBITS/tribits/core/utils/DriveAdvancedTest.cmake:102 (CONFIGURE_FILE):
58:   configure_file Problem configuring file
58: Call Stack (most recent call first):
58:   /ascldap/users/rabartl/Trilinos.base2/Trilinos/TriBITS/tribits/core/utils/DriveAdvancedTest.cmake:383 (SETUP_AND_RUN_TEST_IDX_COPY_FILES_BLOCK)
58:   /ascldap/users/rabartl/Trilinos.base2/BUILDS/GCC-4.8.3/TRIBITS_MPI_DEBUG/test/core/CTestScriptsUnitTests/TriBITS_CTestScripts_TAAT_COPY_FILES_TO_TEST_DIR_bad_file_name.cmake:61 (DRIVE_ADVANCED_TEST)
58: 
58: 
1/1 Test #58: TriBITS_CTestScripts_TAAT_COPY_FILES_TO_TEST_DIR_bad_file_name ...***Failed  Required regular expression not found.Regex=[OVERALL FINAL RESULT: TEST PASSED .TriBITS_CTestScripts_TAAT_COPY_FILES_TO_TEST_DIR_bad_file_name.
]  0.01 sec

And there is no way to control the error handling. It would be nice if the script could continue and and fail gracefully. So what to do?

One idea is to handle the file copy using EXECUTE_PROCESS() with cmake -E copy .... This would allow us to check the return code from that process and then fail the test more gracefully (or about if FAIL_FAST was set). This could be done with one big call to cmake -E <srcDir>/<file0> <srcDir>/<file2> ... <destDir>/. However, I would fear how long that command-line would become if a lot of files were copied. Or, the files could be copied one file at a time with cmake -E <srcDir>/<filei> <destDir>/<filei>. That would result in smaller command-lines but would have more overhead with more system calls.

I think initially, we should just implement the single call to cmake -E copy ... <destDir>/ since that has less overhead and then if we have a problem later with long command-lines, then we can perhaps add an option to break this up into multiple calls.

@fryeguy52, what do you think about this?

[fryeguy52]

Yes I agree that a single call seems better and we can deal with excessively long command-lines if that proves to be a problem later

[bartlettroscoe]

FYI: The tip of the branch:

• https://github.com/TriBITSPub/TriBITS/tree/proto-tribits-system-atdv-84

now has a very robust implementation for failings in creating the dest dir and copying files and I added very strong (nested) testing to ensure the correct behavior. I feel very good about this implementation.

Now I just need to add a check for zero or one SOURCE_DIR and DEST_DIR arguments and write the documentation ...

[bartlettroscoe]

Okay, so that is a bummer, but older versions of CMake (i.e. 2.8.11) do not support cmake -E <file0> <file1> ... <dest_dir>/, they only support cmake -E <file> <dest>. You can see that at:

• http://my.cdash.org/testDetails.php?test=35104905&build=1294628

Therefore, to support CMake 2.8.11, we will have to change this to run EXECUTE_PROCESS( COMMAND cmake -E copy <srcDir>/<filei> <destDir>/<filej> for every file one at a time.

This is another example of the problems we have with having to support older versions of CMake :-(

[bartlettroscoe]

@fryeguy52,

I just created the PR #229 for the completed work for this issue. Can you please take a look at the newer commits (and especially the documentation I wrote in TribitsAddAdvancedTest.cmake)?

Putting this story in review while PR #229 is being reviewed.

[jmgate]

@bartlettroscoe, any update on this issue? Perhaps a tentative timeframe? I'm in the midst of refactoring all of Charon's CMakeLists.txt files, and this looks like it'd be a great feature to take advantage of.

[bartlettroscoe]

This has been done for a whike

[bartlettroscoe]

Could you give it a try and see how it works for you?

[jmgate]

Looks like it worked for one test. Let me try switching over the rest and see how it goes.

[jmgate]

Apparently I spoke too soon—should've done a clean build. Originally I had a CMakeLists.txt file that looked like this:

INCLUDE(TribitsCopyFilesToBinaryDir)
INCLUDE(TribitsAddAdvancedTest)
TRIBITS_COPY_FILES_TO_BINARY_DIR(test_name_dir
  SOURCE_FILES fileA fileB fileC
  EXEDEPS exeName
  NOEXEPREFIX
  )
TRIBITS_ADD_ADVANCED_TEST(
  test_name
  TEST_0 EXEC exeName NOEXEPREFIX DIRECTORY /path/to/where/exeName/lives
    ARGS --i=fileA
    PASS_REGULAR_EXPRESSION "Run completed."
  )

I switched that to

INCLUDE(TribitsAddAdvancedTest)
TRIBITS_ADD_ADVANCED_TEST(
  test_name
  TEST_0 COPY_FILES_TO_TEST_DIR fileA fileB fileC
  TEST_1 EXEC exeName NOEXEPREFIX DIRECTORY /path/to/where/exeName/lives
    ARGS --i=fileA
    PASS_REGULAR_EXPRESSION "Run completed."
  )

Now when I run I see TEST_0 execute just fine—it copies the files into a test_name directory—but TEST_1 doesn't get far because it can't find fileA for input. What should I be doing differently here? Does there need to be some cd into the test_name directory? I'm afraid I can't find any example uses of this COPY_FILES_TO_TEST_DIR anywhere in Trilinos (or TriBITS, for that matter).

Note that the example above only shows a single test in the CMakeLists.txt file. In reality there are plenty of tests, and rather than have all the relevant files listed at the top in TRIBITS_COPY_FILES_TO_BINARY_DIR(), I'd prefer having them grouped with the individual tests using COPY_FILES_TO_TEST_DIR. Am I misunderstanding the use case here? Thanks for your help @bartlettroscoe.

[bartlettroscoe]

@jmgate, I may have missed this use case when adding the automated tests for this new feature. Note that all of the usages of this have used the (very useful) feature to create a new test subdirectory using the OVERALL_WORKING_DIRECTORY [<subdir>|TEST_NAME] option. For that use case, I know it works. But I will add some automated tests for the use case where the OVERALL_WORKING_DIRECTORY option is missing and make sure the correct target directory is being used.

[jmgate]

Thanks @bartlettroscoe—OVERALL_WORKING_DIRECTORY TEST_NAME is what I was missing. This feature looks like it's working for me now.

[bartlettroscoe]

@jmgate,

Note that I added a specific test that should cover the use case were described in this issue with OVERALL_WORKING_DIRECTORY missing and pushed it in the commit f8d7208. That seemed to show that it worked as it should. But I think that might be because I may have fixed this in the TriBITS commit 1dfc788 that has not been snapshotted to Trilinos develop yet. I will update the TriBITS snapshot in Trilinos so that no one else hits this use case.

But really most tests should use OVERALL_WORKING_DIRECTORY [<subdir>|TEST_NAME] since it creates more robust tests for several reasons.

Sorry for the trouble this caused.

[jmgate]

No worries @bartlettroscoe. I've converted all of Charon's CMakeLists.txt files to use OVERALL_WORKING_DIRECTORY TEST_NAME now, so I think we're good to go.