Summary:	ASTERISK-23369: [patch] Testsuite: Timeout shutting down asterisk should result in failed test
Reporter:	Corey Farrell (coreyfarrell)	Labels:
Date Opened:	2014-02-24 16:47:59.000-0600	Date Closed:	2014-11-04 09:16:33.000-0600
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Tests/testsuite
Versions:	SVN	Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) testsuite-asterisk-signal-abrt.patch
Description:	asterisk.stop() first attempts to shutdown asterisk gracefully.  If this does not complete after 10 seconds signal 9 is used.  I believe this should be signal 6 - ABRT and core dump. * Backtrace of all threads may give clues to why graceful shutdown is not successful. * Any test that fails to hangup channels needs to be fixed. * If the test did hangup all channels, then we have a reference leak. It would also be helpful to increase the graceful shutdown timeout from 10 to 20 seconds.  While testing testsuite ABRT I found a few tests that just needed a few more seconds to shutdown gracefully, mostly when more than 1 instance of asterisk was in play.
Comments:	By: Corey Farrell (coreyfarrell) 2014-02-24 16:53:53.494-0600 This patch causes 39 test failures, producing 40 backtraces. By: Matt Jordan (mjordan) 2014-02-28 11:38:33.228-0600 So... while I agree that this should be an option, I'm not convinced it should be the default. Tests should clean up after themselves, and we should fix them - and this is a useful tool to find such tests. Tests that don't clean up properly could be a sign of a larger problem, but it may also not indicate any problem in Asterisk - for example, a test may have just decided to not hang up its channels. A test not cleaning up after itself is not as critical as an actual crash in Asterisk, or a functionality problem - and having this on by default would make it so those problems appear as equivalent. How about a config parameter in the top most test-config.yaml file? That would let you enable the setting for an entire testsuite run. By: Corey Farrell (coreyfarrell) 2014-02-28 13:07:24.092-0600 I mostly agree with your comments.  Part of the reason I didn't create an option in test-config.yaml is I didn't (don't) know how.  I'll have time to look into that next week.  I think this option should be used to find and fix all tests that have to signal kill asterisk.  After we've fixed existing tests I do think this should be enabled by default, or even hard-coded on like the attached patch. A test not cleaning up after itself as not critical as an issue in Asterisk, but unfortunately the testsuite only supports pass, fail or skip.  My fear is that nobody looks at anything for passed tests.  I agree that if the test is broken this is not the same fail as asterisk crashing, but it is still a fail. By: Corey Farrell (coreyfarrell) 2014-11-04 09:16:33.339-0600 ASTERISK-24379 effectively resolves this issue.  Any instance of Asterisk that doesn't shutdown gracefully will leak references, if REF_DEBUG is enabled the test will fail due to leaks.