Skip to content
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

Detect when a unit test child process crashes (RIPD-1592): #2415

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@seelabs
Copy link
Contributor

seelabs commented Feb 28, 2018

When a test suite starts and ends, it informs the parent process. If the parent
has received a start message without a matching end message it reports that a
child may have crashed in that suite.

@seelabs

This comment has been minimized.

Copy link
Contributor Author

seelabs commented Feb 28, 2018

Note: there is no unit test for this (a unit test that actually crashes the process is a bad idea). I tested this by inserting code that crashed a unit test and (*reinterpret_cast<int*>(0)=1) and checked the suite was reported as crashed.

@ximinez

This comment has been minimized.

Copy link
Contributor

ximinez commented Feb 28, 2018

Could you make that a manual test?

testcase ("Detect Crash");
// Crash the process. This is used to test that the multi-process
// unit test will correctly report the crash.
*reinterpret_cast<volatile int*>(0) = 0;

This comment has been minimized.

Copy link
@ximinez

ximinez Mar 1, 2018

Contributor

Apparently this throws an actual exception in Windows builds:

$ ./build/cmake/msvc/Release/rippled --unittest=DetectCrash --unittest-jobs=6   
0> beast.unit_test.DetectCrash Detect Crash
0> #1 failed: unhandled exception
5.7s, 1 suite, 1 case, 1 test total, 1 failure

$ ./build/cmake/msvc/Debug/rippled --unittest=DetectCrash --unittest-jobs=6
0> beast.unit_test.DetectCrash Detect Crash
0> #1 failed: unhandled exception
9.8s, 1 suite, 1 case, 1 test total, 1 failure

$ ./build/cmake/msvc/Debug/rippled --unittest=DetectCrash
beast.unit_test.DetectCrash Detect Crash
#1 failed: unhandled exception
142ms, 1 suite, 1 case, 1 test total, 1 failure

In Linux, I see the expected "may have crashed message".

$ ./build/cmake/gcc.debug/rippled --unittest=DetectCrash --unittest-jobs=6
0> beast.unit_test.DetectCrash Detect Crash
602ms, 0 suites, 0 cases, 0 tests total, 0 failures

Suite: beast.unit_test.DetectCrash failed to complete. The child process may have crashed.

Changing this to a "simple" std::terminate(), however, seems to work better in both environments, though Windows displays an Abort/Retry/Ignore dialog.

$ ./build/cmake/msvc/Debug/rippled --unittest=DetectCrash --unittest-jobs=6
1> beast.unit_test.DetectCrash Detect Crash
1.6s, 0 suites, 0 cases, 0 tests total, 0 failures

Suite: beast.unit_test.DetectCrash failed to complete. The child process may have crashed.

ximinez@8a7ae1e

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 1, 2018

Author Contributor

Thanks for finding that. I took your fix and pushed. I'm a little curious as to what kind of exception was thrown. Next time I boot up windows I might run a little experiment.

@ripplelabs-jenkins

This comment has been minimized.

Copy link
Collaborator

ripplelabs-jenkins commented Mar 1, 2018

Jenkins Build Summary

Built from this commit

Built at 20180309 - 19:53:14

Test Results

Build Type Log Result Status
docs logfile 1 cases, 0 failed, t: 3s PASS
clang.debug.unity logfile 1003 cases, 0 failed, t: 247s PASS
coverage logfile 1003 cases, 0 failed, t: 603s PASS
clang.debug.nounity logfile 1001 cases, 0 failed, t: 189s PASS
clang.debug.unity,
PARALLEL_TESTS=false
logfile 1003 cases, 0 failed, t: 466s PASS
gcc.debug.unity logfile 1003 cases, 0 failed, t: 237s PASS
gcc.debug.nounity logfile 1001 cases, 0 failed, t: 307s PASS
clang.release.unity logfile 1002 cases, 0 failed, t: 325s PASS
msvc.debug logfile 999 cases, 0 failed, t: 551s PASS
gcc.release.unity logfile 1002 cases, 0 failed, t: 344s PASS
gcc.debug.unity
-Dstatic=true
logfile 1003 cases, 0 failed, t: 237s PASS
msvc.debug,
PROJECT_NAME=rippled_classic
logfile 999 cases, 0 failed, t: 1127s PASS
msvc.release logfile 998 cases, 0 failed, t: 495s PASS
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 1, 2018

Codecov Report

Merging #2415 into develop will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #2415      +/-   ##
==========================================
- Coverage    70.32%   70.3%   -0.02%     
==========================================
  Files          702     702              
  Lines        53429   53431       +2     
==========================================
- Hits         37573   37567       -6     
- Misses       15856   15864       +8
Impacted Files Coverage Δ
src/ripple/app/main/Main.cpp 35.96% <0%> (-0.32%) ⬇️
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/protocol/impl/STVar.cpp 85.71% <0%> (-2.6%) ⬇️
src/ripple/protocol/impl/Serializer.cpp 70.06% <0%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8648440...3bab384. Read the comment docs.

@ximinez

ximinez approved these changes Mar 2, 2018

Copy link
Contributor

ximinez left a comment

👍

this->os_.flush();
break;
case MessageType::test_start:
running_suites_.insert(s);

This comment has been minimized.

Copy link
@miguelportilla

miguelportilla Mar 9, 2018

Member

Suggestion: running_suites_.insert(std::move(s));

This comment has been minimized.

Copy link
@seelabs

seelabs Mar 9, 2018

Author Contributor

Fixed.

@miguelportilla

This comment has been minimized.

Copy link
Member

miguelportilla commented Mar 9, 2018

LGTM 👍

@seelabs seelabs force-pushed the seelabs:detect-unittest-crash branch from 420809c to 2492692 Mar 9, 2018

Detect when a unit test child process crashes (RIPD-1592):
When a test suite starts and ends, it informs the parent process. If the parent
has received a start message without a matching end message it reports that a
child may have crashed in that suite.

@seelabs seelabs force-pushed the seelabs:detect-unittest-crash branch from 2492692 to 3bab384 Mar 9, 2018

@seelabs seelabs added the Passed label Mar 9, 2018

@seelabs

This comment has been minimized.

Copy link
Contributor Author

seelabs commented Mar 15, 2018

In 1.0.0-b2

@seelabs seelabs closed this Mar 15, 2018

@seelabs seelabs deleted the seelabs:detect-unittest-crash branch Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.