Project

General

Profile

Bug #1795

limit test binaries to only one MPI rank where necessary/appropriate

Added by Mark Abraham about 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
testing
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

While testing 5.1-rc1, I observed a handful of commandline-test cases fail on two BlueGene/Q machines.

Four parse_common_args tests rely on the test machinery touching a dummy file, and then that function checks for existence of that file as part of the test. This does not work reliably on those machines - presumably parallel file-system buffering accepts the writes but has not committed them when we later check for existence of the files. The test code can probably be fixed with the newly introduced IFileInputRedirector, but currently this is awkward to do. We could either add a new PCA_USE_TEST_FILE_INPUT_REDIRCTOR (ugly to have release code check for things only used in testing), or somehow parameterize parse_common_args on a redirector object (ugly because it is currently a C-callable free function). Neither of those are things we really want to do in a release branch, nor need to do for BG/Q or in the interests of the wider project.

Perhaps similarly, many CommandLineHelpModule and CommandLineModuleManager test cases fail in ways that a quick scan suggests are related to file creation.

For 5.1, I'll disable all the above for BG/Q, since it's just not functionality we need to test there. I can't think of a more sensible criterion for disabling - but maybe we want to introduce some kind of flag that we can use when we can judge that we should not "test problematic stuff that's anyway not very relevant when running on big iron?"

In master, with callers of parse_common_args now C++, we can probably do something sexier that lets test code parameterize away the dependency on a real file system.

Associated revisions

Revision c8047a4b (diff)
Added by Teemu Murtola over 4 years ago

Disable multiple ranks for non-MPI unit tests

Make tests not explicitly declared in CMake as supporting MPI only run
on a single MPI rank. This is already the case for the 'test' and
'check' targets, but manually it is possible to run them with multiple
ranks. Now all other ranks except the master will exit, and only master
will run the tests. This avoids potential race conditions in tests not
designed for concurrent execution, e.g., related to file system access.
Some cleanup of the related CMake macros.

Closes #1795

Change-Id: I69e88ba3419cce96eb5b0c7e145643accc65533d

History

#1 Updated by Gerrit Code Review Bot about 5 years ago

Gerrit received a related patchset '1' for Issue #1795.
Uploader: Mark Abraham ()
Change-Id: Ic376f3b2f6127e4fee9d9b51d8982b973c357813
Gerrit URL: https://gerrit.gromacs.org/4947

#2 Updated by Teemu Murtola about 5 years ago

You cannot forbid all file system access from the tests... That would make it impossible to write tests for 95% or more of our file I/O routines, nearly all of which operate purely on file names or FILE *. And refactoring any of that to use something else without the help of automatic tests is not attractive.

These particular tests we can fix with some hacks, but that does not solve the general problem that, e.g., the fileio tests in master cannot work without working on physical files (at least, not without rewriting the whole code that is being tested).

The benefit of accelerated development for code that is automatically tested is in my mind much greater than potentially failing tests on some exotic systems where no one is really touching the code that is being tested by those tests.

#3 Updated by Mark Abraham about 5 years ago

Teemu Murtola wrote:

You cannot forbid all file system access from the tests... That would make it impossible to write tests for 95% or more of our file I/O routines, nearly all of which operate purely on file names or FILE *. And refactoring any of that to use something else without the help of automatic tests is not attractive.

All of my comments clearly pertain to a subset of the tests applicable to parse_common_args and CommandLineHelp modules. Not to "all file system access."

These particular tests we can fix with some hacks, but that does not solve the general problem that, e.g., the fileio tests in master cannot work without working on physical files (at least, not without rewriting the whole code that is being tested).

Right. The tests I have hacked off are those that are fragile, but also those that don't really need a real file system. Once 5.1 is out then I will try out the fileio tests in master branch to see what issues we have on BG/Q.

The benefit of accelerated development for code that is automatically tested is in my mind much greater than potentially failing tests on some exotic systems where no one is really touching the code that is being tested by those tests.

... which is why I suggested we consider creating a flag so that we can test things on a real file system when we have to (probably fileio tests), not test such things where it is unduly painful to do so (probably fileio tests on BG/Q and ilk), and test without requiring a real file system when we can (e.g. there's already a TODO in commandline/tests/cmdlinehelpmodule.cpp noting that needing to write links.dat is something to work on).

For example, the four parse_common_args tests just need to be able to use a mock-like thing (perhaps IFileInputRedirector) that returns true for fileExists() when programmed to do so, just like we already have in src/testutils/testfileredirector.cpp. Those don't need a real file system. (And yes, I do appreciate that many of the problems pre-date your efforts, and that it's sucky not to be able to have CI also on such systems... we just can't ship a test set that is known to fail in ways that are not actually problems!)

#4 Updated by Teemu Murtola about 5 years ago

Mark Abraham wrote:

All of my comments clearly pertain to a subset of the tests applicable to parse_common_args and CommandLineHelp modules. Not to "all file system access."

But what you seem to imply that any test that first writes to a file and then reads it is problematic. And all my comments pertain to such tests; without this ability, it will be
  1. Impossible to test any code whose primary purpose is to write files (since the only way to test that it happened correctly is to read those files), and
  2. Tedious to test code that reads files, if the only allowed way to do so is to set up static files in the source tree that are then read by the test. In particular if the test needs multiple files, and different files for each tests, this leads to an explosion of files in the source tree, and makes it unduly complex to set up and understand such tests.

As for the other stuff, I've said it before and I can say it again: we would get (at least) 99% of the benefit of the tests even if it was impossible for anyone but the developers (and Jenkins) to run them on their primary development environment. It's quite a niche use case for some user to find a real issue using the unit tests, so we should not be making the tests unnecessarily complex to write and maintain (e.g., by requiring that every test that needs to manage files is #ifdef'd out on some esoteric systems, or requiring that they would not be written at all, or requiring all existing code to be refactored to support testing without a file system) just to support such a niche case.

#5 Updated by Mark Abraham about 5 years ago

Yes, I know all of this. Doing no testing, and letting all tests do anything while requiring all tests work everywhere are both unworkable approaches, and I have suggested neither of those.

I've already twice suggested we consider defining a subset of the unit tests that are not intended to run everywhere. Those that can run without needing to write files can be enabled by default. It seems that those that need to write files need to be enabled only under some circumstances. Is that what you consider "unnecessarily complex?"

As you say, 99% of the value of the tests are obtained even if we would run the tests only by devs on normal machines, and under Jenkins. The workable solutions are
  1. to maintain a separation of the unit tests into those that run everywhere and those that don't (maybe too much work to define the criteria, the situations that meet it, and the ongoing logic to implement it),
  2. to de-support the unit tests on non-Jenkins systems, answer emails when users and devs find broken tests on such machines, and get a bad reputation for not passing our own tests, or
  3. to disable the unit tests on non-Jenkins systems (which still requires that we define that set and implement one-time logic to enforce it)

If we don't like 1, then I think we need to go with 3.

#6 Updated by Teemu Murtola about 5 years ago

Fine, let's go with 1. But the title of the issue is very misleading if you do not actually mean that it should happen like that.

But I still dislike your approach of sprinkling the CMake and source code with mysterious GMX_TARGET_BGQ conditionals. That is just unmaintainable, both in determining where they need to go, and then changing them everywhere if someone finds another, unrelated system that has similar issues. Rather, I suggest that we either run all tests from a subdirectory, or none, and concentrate all the logic in TestMacros.cmake. If you want to have something quickly in release-5-1, I'm fine with just putting a single if (NOT GMX_TARGET_BGQ) in the appropriate place, but what we really should do is to have a more generically named parameter for gmx_add_unit_test() (NEEDS_RELIABLE_FILE_SYSTEM or something else) and then have TestMacros.cmake be the single place that knows when such tests can be run).

#7 Updated by Mark Abraham about 5 years ago

  • Subject changed from unit tests should not rely on file creation to separate unit tests that should always work from those that have possible problems

#8 Updated by Gerrit Code Review Bot about 5 years ago

Gerrit received a related patchset '1' for Issue #1795.
Uploader: Mark Abraham ()
Change-Id: I69e88ba3419cce96eb5b0c7e145643accc65533d
Gerrit URL: https://gerrit.gromacs.org/4957

#9 Updated by Mark Abraham about 5 years ago

For the record, before FileNameOption* tests used the input redirector (ie. 0d9d2c84de9), they had similar issues. (I'm pretty sure the exception is thrown after a check for file existence fails.)

...
[       OK ] FileNameOptionManagerTest.AcceptsMissingRequiredInputFileIfSpecified (1 ms)
[ RUN      ] FileNameOptionManagerTest.AddsMissingExtensionBasedOnExistingFile
Test failures from rank 0:
../src/gromacs/options/tests/filenameoptionmanager.cpp:265: Failure
Expected: assigner.appendValue(inputValue) doesn't throw an exception.
  Actual: it throws.
Exception details:
File '/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionBasedOnExistingFile' does not exist or is not accessible.
The following extensions were tried to complete the file name:
  .xtc, .trr, .cpt, .gro, .g96, .pdb, .tng
../src/gromacs/options/tests/filenameoptionmanager.cpp:270: Failure
Value of: value
  Actual: "" 
Expected: filename
Which is: "/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionBasedOnExistingFile.trr" 
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionBasedOnExistingFile (60 ms)
[ RUN      ] FileNameOptionManagerTest.AddsMissingExtensionForRequiredDefaultNameBasedOnExistingFile
Test failures from rank 0:
../src/gromacs/options/tests/filenameoptionmanager.cpp:291: Failure
Expected: options_.finish() doesn't throw an exception.
  Actual: it throws.
Exception details:
Invalid input values
  In option f
    No file name was provided, and the default file '/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForRequiredDefaultNameBasedOnExistingFile' does not exist or is not accessible
.
The following extensions were tried to complete the file name:
  .xtc, .trr, .cpt, .gro, .g96, .pdb, .tng
../src/gromacs/options/tests/filenameoptionmanager.cpp:293: Failure
Value of: value
  Actual: "/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForRequiredDefaultNameBasedOnExistingFile.xtc" 
Expected: filename
Which is: "/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForRequiredDefaultNameBasedOnExistingFile.trr" 
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForRequiredDefaultNameBasedOnExistingFile (19 ms)
[ RUN      ] FileNameOptionManagerTest.AddsMissingExtensionForOptionalDefaultNameBasedOnExistingFile
Test failures from rank 0:
../src/gromacs/options/tests/filenameoptionmanager.cpp:313: Failure
Expected: options_.finish() doesn't throw an exception.
  Actual: it throws.
Exception details:
Invalid input values
  In option f
    No file name was provided, and the default file '/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForOptionalDefaultNameBasedOnExistingFile' does not exist or is not accessible
.
The following extensions were tried to complete the file name:
  .xtc, .trr, .cpt, .gro, .g96, .pdb, .tng
../src/gromacs/options/tests/filenameoptionmanager.cpp:315: Failure
Value of: value
  Actual: "/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForOptionalDefaultNameBasedOnExistingFile.xtc" 
Expected: filename
Which is: "/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForOptionalDefaultNameBasedOnExistingFile.trr" 
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForOptionalDefaultNameBasedOnExistingFile (19 ms)
[ RUN      ] FileNameOptionManagerTest.AddsMissingExtensionForRequiredFromDefaultNameOptionBasedOnExistingFile
Test failures from rank 0:
../src/gromacs/options/tests/filenameoptionmanager.cpp:338: Failure
Expected: options_.finish() doesn't throw an exception.
  Actual: it throws.
Exception details:
Invalid input values
  In option f
    Required option was not provided, and the default file 'foo' does not exist or is not accessible.
The following extensions were tried to complete the file name:
  .xtc, .trr, .cpt, .gro, .g96, .pdb, .tng
../src/gromacs/options/tests/filenameoptionmanager.cpp:340: Failure
Value of: value
  Actual: "foo.xtc" 
Expected: filename
Which is: "/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForRequiredFromDefaultNameOptionBasedOnExistingFile.trr" 
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForRequiredFromDefaultNameOptionBasedOnExistingFile (19 ms)
[ RUN      ] FileNameOptionManagerTest.AddsMissingExtensionForOptionalFromDefaultNameOptionBasedOnExistingFile
Test failures from rank 0:
../src/gromacs/options/tests/filenameoptionmanager.cpp:364: Failure
Expected: options_.finish() doesn't throw an exception.
  Actual: it throws.
Exception details:
Invalid input values
  In option f
    No file name was provided, and the default file 'foo' does not exist or is not accessible.
The following extensions were tried to complete the file name:
  .xtc, .trr, .cpt, .gro, .g96, .pdb, .tng
../src/gromacs/options/tests/filenameoptionmanager.cpp:366: Failure
Value of: value
  Actual: "foo.xtc" 
Expected: filename
Which is: "/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/options/tests/Testing/Temporary/FileNameOptionManagerTest_AddsMissingExtensionForOptionalFromDefaultNameOptionBasedOnExistingFile.trr" 
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForOptionalFromDefaultNameOptionBasedOnExistingFile (19 ms)
[----------] 14 tests from FileNameOptionManagerTest (201 ms total)

[----------] 8 tests from FileNameOptionTest
[ RUN      ] FileNameOptionTest.HandlesRequiredDefaultValueWithoutExtension
[       OK ] FileNameOptionTest.HandlesRequiredDefaultValueWithoutExtension (0 ms)
[ RUN      ] FileNameOptionTest.HandlesRequiredOptionWithoutValue
[       OK ] FileNameOptionTest.HandlesRequiredOptionWithoutValue (0 ms)
[ RUN      ] FileNameOptionTest.HandlesOptionalUnsetOption
[       OK ] FileNameOptionTest.HandlesOptionalUnsetOption (0 ms)
[ RUN      ] FileNameOptionTest.HandlesOptionalDefaultValueWithoutExtension
[       OK ] FileNameOptionTest.HandlesOptionalDefaultValueWithoutExtension (0 ms)
[ RUN      ] FileNameOptionTest.HandlesRequiredCustomDefaultExtension
[       OK ] FileNameOptionTest.HandlesRequiredCustomDefaultExtension (0 ms)
[ RUN      ] FileNameOptionTest.HandlesOptionalCustomDefaultExtension
[       OK ] FileNameOptionTest.HandlesOptionalCustomDefaultExtension (0 ms)
[ RUN      ] FileNameOptionTest.GivesErrorOnUnknownFileSuffix
[       OK ] FileNameOptionTest.GivesErrorOnUnknownFileSuffix (1 ms)
[ RUN      ] FileNameOptionTest.GivesErrorOnInvalidFileSuffix
[       OK ] FileNameOptionTest.GivesErrorOnInvalidFileSuffix (0 ms)
[----------] 8 tests from FileNameOptionTest (9 ms total)

[----------] Global test environment tear-down
[==========] 22 tests from 2 test cases ran. (214 ms total)
[  PASSED  ] 17 tests.
[  FAILED  ] 5 tests, listed below:
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionBasedOnExistingFile
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForRequiredDefaultNameBasedOnExistingFile
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForOptionalDefaultNameBasedOnExistingFile
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForRequiredFromDefaultNameOptionBasedOnExistingFile
[  FAILED  ] FileNameOptionManagerTest.AddsMissingExtensionForOptionalFromDefaultNameOptionBasedOnExistingFile

 5 FAILED TESTS

#10 Updated by Mark Abraham about 5 years ago

In release-5-1 HEAD, only cases in commandline-tests fail (but the pargs tests do so by throwing an uncaught exception like

[ RUN      ] ParseCommonArgsTest.HandlesCompressedFiles

-------------------------------------------------------
Program:     commandline-test, VERSION 5.1-rc1-dev-20150804-bc51e73
Source file: ../src/gromacs/commandline/cmdlineparser.cpp (line 234)
Function:    gmx::CommandLineParser::parse(int *, char *[])

Error in user input:
Invalid command-line options
  In command-line option -g
    File
    '/homea/slbio/slbio013/git/r51/build-cmake-xlc-single-release/src/gromacs/commandline/tests/Testing/Temporary/ParseCommonArgsTest_HandlesCompressedFiles.gro.Z'
    does not exist or is not accessible.
    The following extensions were tried to complete the file name:
      .gro

When I run with --gtest_filter=-\*ParseCommonArgs\* I get undesirable behaviour from only

[ RUN      ] CommandLineModuleManagerTest.RunsModuleHelp
Test failures from rank 1:
../src/gromacs/commandline/tests/cmdlinemodulemanager.cpp:92: Failure
Actual function call count doesn't match EXPECT_CALL(mod1, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[       OK ] CommandLineModuleManagerTest.RunsModuleHelp (3 ms)
[ RUN      ] CommandLineModuleManagerTest.RunsModuleHelpWithDashH
Test failures from rank 1:
../src/gromacs/commandline/tests/cmdlinemodulemanager.cpp:109: Failure
Actual function call count doesn't match EXPECT_CALL(mod1, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[       OK ] CommandLineModuleManagerTest.RunsModuleHelpWithDashH (3 ms)
[ RUN      ] CommandLineModuleManagerTest.RunsModuleHelpWithDashHWithSingleModule
Test failures from rank 1:
../src/gromacs/commandline/tests/cmdlinemodulemanager.cpp:126: Failure
Actual function call count doesn't match EXPECT_CALL(mod, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[       OK ] CommandLineModuleManagerTest.RunsModuleHelpWithDashHWithSingleModule (2 ms)
[ RUN      ] CommandLineModuleManagerTest.HandlesConflictingBinaryAndModuleNames
[       OK ] CommandLineModuleManagerTest.HandlesConflictingBinaryAndModuleNames (2 ms)
[----------] 5 tests from CommandLineModuleManagerTest (29 ms total)

[----------] 4 tests from CommandLineHelpWriterTest
[ RUN      ] CommandLineHelpWriterTest.HandlesOptionTypes
[       OK ] CommandLineHelpWriterTest.HandlesOptionTypes (61 ms)
[ RUN      ] CommandLineHelpWriterTest.HandlesLongFileOptions
[       OK ] CommandLineHelpWriterTest.HandlesLongFileOptions (30 ms)
[ RUN      ] CommandLineHelpWriterTest.HandlesLongOptions
[       OK ] CommandLineHelpWriterTest.HandlesLongOptions (29 ms)
[ RUN      ] CommandLineHelpWriterTest.HandlesMultipleSections
[       OK ] CommandLineHelpWriterTest.HandlesMultipleSections (30 ms)
[----------] 4 tests from CommandLineHelpWriterTest (153 ms total)

[----------] 3 tests from CommandLineHelpModuleTest
[ RUN      ] CommandLineHelpModuleTest.PrintsGeneralHelp
[       OK ] CommandLineHelpModuleTest.PrintsGeneralHelp (26 ms)
[ RUN      ] CommandLineHelpModuleTest.PrintsHelpOnTopic
Test failures from rank 1:
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:100: Failure
Actual function call count doesn't match EXPECT_CALL(topic, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[       OK ] CommandLineHelpModuleTest.PrintsHelpOnTopic (22 ms)
[ RUN      ] CommandLineHelpModuleTest.ExportsHelp
Test failures from rank 0:
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:157: Failure
Expected: rc = manager().run(args.argc(), args.argv()) doesn't throw an exception.
  Actual: it throws.
Exception details:
Could not open file 'links.dat'
  Reason: No such file or directory
  (call to fopen() returned error code 2)
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:150: Failure
Actual function call count doesn't match EXPECT_CALL(mod2, initOptions(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:149: Failure
Actual function call count doesn't match EXPECT_CALL(mod1, initOptions(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:151: Failure
Actual function call count doesn't match EXPECT_CALL(topic1, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:152: Failure
Actual function call count doesn't match EXPECT_CALL(sub1, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:153: Failure
Actual function call count doesn't match EXPECT_CALL(sub2, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:154: Failure
Actual function call count doesn't match EXPECT_CALL(sub3, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:155: Failure
Actual function call count doesn't match EXPECT_CALL(topic2, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
Test failures from rank 1:
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:150: Failure
Actual function call count doesn't match EXPECT_CALL(mod2, initOptions(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:149: Failure
Actual function call count doesn't match EXPECT_CALL(mod1, initOptions(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:151: Failure
Actual function call count doesn't match EXPECT_CALL(topic1, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:152: Failure
Actual function call count doesn't match EXPECT_CALL(sub1, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:153: Failure
Actual function call count doesn't match EXPECT_CALL(sub2, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:154: Failure
Actual function call count doesn't match EXPECT_CALL(sub3, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
../src/gromacs/commandline/tests/cmdlinehelpmodule.cpp:155: Failure
Actual function call count doesn't match EXPECT_CALL(topic2, writeHelp(_))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] CommandLineHelpModuleTest.ExportsHelp (47 ms)

So it really does look like a check for a recently made file doesn't pass

#11 Updated by Teemu Murtola about 5 years ago

Something is not right. The messages from Google Mock (and the output before them) indicate that you are running the test with two MPI ranks, which is was not designed to handle. Are you sure that you are not just observing the two ranks racing to create and delete the same files? This would also match the symptoms, since our MPI output routines have the side effect that rank 0 runs a bit behind the other ranks.

#12 Updated by Mark Abraham about 5 years ago

Teemu Murtola wrote:

Something is not right. The messages from Google Mock (and the output before them) indicate that you are running the test with two MPI ranks, which is was not designed to handle. Are you sure that you are not just observing the two ranks racing to create and delete the same files? This would also match the symptoms, since our MPI output routines have the side effect that rank 0 runs a bit behind the other ranks.

Good point. I was running with two ranks (trying to cater for the MPI integration tests) but as you say, these tests aren't designed for that. Running with only one rank does avoid the problem. Sorry for the false alarm :-( Thus we probably don't need to change anything for 5.1.

It might be nice to work around people running such a serial binary with mpirun. If compiled with MPI support, then we might default the test machinery to calling MPI_Init, and having non-zero ranks exit, and maybe have rank 0 note that other ranks did nothing?

#13 Updated by Teemu Murtola about 5 years ago

That should be doable (in master). It requires some mechanism to flag which of the test binaries contain proper MPI tests, though. Any preference on how that should work? It's possible to have it as part of the gmx_add_unit_test() signature in CMake, and probably also as some macro placed in source files. But it will still mean that mixing MPI and non-MPI-aware tests in the same binary can cause issues, so we may want to split MPI-aware tests to a completely separate binary.

#14 Updated by Mark Abraham about 5 years ago

  • Subject changed from separate unit tests that should always work from those that have possible problems to limit test binaries to only one MPI rank where necessary/appropriate
  • Target version changed from 5.1 to future

There should probably be some CMake-level machinery (if it's not there already) for using the MPIEXEC_* flags that I did some work on in ~January. Thus there could/should be some separation at that level. So doing the split at the level of whole test binaries seems both useful and convenient. This might make it possible to propagate something into the src/testutils code to make test binaries do the right thing when run with more than one rank. (Perhaps similarly if something similar is every relevant with more than one thread, but seems unlikely.)

It is useful that one can run the stand-alone test binary, but most people will do so via make test, so that is the high-value target.

#15 Updated by Mark Abraham about 5 years ago

The normal unit tests don't need more than one rank (and as here, might test features / use test harnesses that can't use more than one rank), but we will certainly have test cases that expect more than one rank (e.g. multi-simulation requires >1 rank, high-level DD testing certainly will need a wide range of possible ranks, anticipated replacements for parts of gmxtest.pl will want multiple ranks).

In general, for running MPI-enabled test binaries, one needs to call via an mpirun-like launcher. Some MPI runtimes let you run the plain binary and if so gives you a single-rank run, but in at least some cases (BG/Q and probably all Crays) you need to use the launcher.

So, I think make check for an MPI build should expect to use the MPIEXEC_* parameters "found" in FindMPI.cmake for launching the test binaries in that case, along the lines of what we have for the mdrun MPI integration tests in src/testutils/TestMacros.cmake now. By default, make check will run test binaries with a single rank, but when we register a test binary we should have an interface to suggest a range of suitable numbers of MPI ranks, and a CMake flag to specify the maximum number of MPI ranks that can be created while testing (which will have to correspond to the user's job script that runs make check). This means it is possible for us to set things up so that a non-expert can test an MPI build with a correct set of CMake flags and a call to make check. That will need more documentation also, of course.

Naturally, all of that is for master branch.

#16 Updated by Teemu Murtola almost 5 years ago

  • Status changed from New to Fix uploaded
  • Target version changed from future to 2016

#17 Updated by Teemu Murtola over 4 years ago

  • Status changed from Fix uploaded to Resolved

#18 Updated by Teemu Murtola over 4 years ago

  • Status changed from Resolved to Closed
  • Assignee set to Teemu Murtola

Also available in: Atom PDF