DrylandEcology / SOILWAT2

An ecosystem water balance simulation model
GNU General Public License v3.0
6 stars 2 forks source link

New unit tests on master are intermittently failing on travis and appveyor #174

Closed dschlaep closed 6 years ago

dschlaep commented 6 years ago
dschlaep commented 6 years ago

For instance, travis-ci (https://travis-ci.org/DrylandEcology/SOILWAT2/jobs/387769650):

[ RUN      ] SWFlowTempTest.MainSoilTemperatureFunction
 Depth of Layers 20.000000, MaxDepth 1.000000test/test_SW_Flow_lib_temp.cc:535: Failure
Death test: soil_temperature(airTemp, pet, aet, biomass, swc, swc_sat, bDensity, width, oldsTemp, sTemp, surfaceTemp, nlyrs, fc, wp, bmLimiter, t1Param1, t1Param2, t1Param3, csParam1, csParam2, shParam, snowdepth, sTconst, deltaX, theMaxDepth2, nRgr, snow, &ptr_stError)
    Result: failed to die.
 Error msg:
[  DEATH   ] 
[  FAILED  ] SWFlowTempTest.MainSoilTemperatureFunction (8 ms)
dschlaep commented 6 years ago

Same problem on appveyor:

[ RUN      ] SWFlowTempTest.MainSoilTemperatureFunction
 Depth of Layers 20.000000, MaxDepth 1.000000
[WARNING] googletest/googletest/src/gtest-death-test.cc:836:: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test couldn't detect the number of threads.
test/test_SW_Flow_lib_temp.cc:535: Failure
Death test: soil_temperature(airTemp, pet, aet, biomass, swc, swc_sat, bDensity, width, oldsTemp, sTemp, surfaceTemp, nlyrs, fc, wp, bmLimiter, t1Param1, t1Param2, t1Param3, csParam1, csParam2, shParam, snowdepth, sTconst, deltaX, theMaxDepth2, nRgr, snow, &ptr_stError)
    Result: failed to die.
 Error msg:
[  DEATH   ] 
[  FAILED  ] SWFlowTempTest.MainSoilTemperatureFunction (19 ms)
[----------] 6 tests from SWFlowTempTest (121 ms total)

Restart and it works.

CaitlinA commented 6 years ago

I am having trouble determining if this is a DEATH_TEST error because we are using a CI or if this indicated that the SOILWAT2 model really fails to fail when we expect it to.

The tests consistently pass on my local.

What is your take on this?

On Tue, Jun 5, 2018 at 8:40 AM, daniel notifications@github.com wrote:

Same problem on appveyor:

[ RUN ] SWFlowTempTest.MainSoilTemperatureFunction Depth of Layers 20.000000, MaxDepth 1.000000 [WARNING] googletest/googletest/src/gtest-death-test.cc:836:: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test couldn't detect the number of threads. test/test_SW_Flow_lib_temp.cc:535: Failure Death test: soil_temperature(airTemp, pet, aet, biomass, swc, swc_sat, bDensity, width, oldsTemp, sTemp, surfaceTemp, nlyrs, fc, wp, bmLimiter, t1Param1, t1Param2, t1Param3, csParam1, csParam2, shParam, snowdepth, sTconst, deltaX, theMaxDepth2, nRgr, snow, &ptr_stError) Result: failed to die. Error msg: [ DEATH ] [ FAILED ] SWFlowTempTest.MainSoilTemperatureFunction (19 ms) [----------] 6 tests from SWFlowTempTest (121 ms total)

Restart and it works.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/DrylandEcology/SOILWAT2/issues/174#issuecomment-394757902, or mute the thread https://github.com/notifications/unsubscribe-auth/AM0e_-dllJgH0s0B8xi1D6sAk_33U2paks5t5qZVgaJpZM4UX4KU .

-- Caitlin Andrews Ecologist Southwest Biological Science Center U.S. Geological Survey Mobile: 802-922-3494 Office: 928-556-7036

dschlaep commented 6 years ago

It appears that you are testing with that line 535 that the function soil_temperature_init experiences that LT(theMaxDepth, st->depths[nlyrs - 1]) is met. Depending on the value of ptr_stError the code calls return; (the return type of the function is void) or alternatively enters LogError with LOGFATAL and thus calls sw_error with -1 which calls exit(-1).

Because this line uses EXPECT_DEATH, the expectation is met if the function exists with non-zero (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#death-tests).

The unit tests code does not make it explicit whether ptr_stError is TRUE or FALSE when entering line 535. I guess that it is FALSE based on a quick glance of the code (I suggest that ptr_stError = XXX; is added just before to explicitly state the intent). In any case, if FALSE, then soil_temperature_init returns void. I guess that this is not clearly and consistently identifiable as death.

Also note that any death expectations should be grouped in separate TESTS() and named accordingly ...DeathTest... (see documentation and the death tests that we wrote together test_rands.cc)