Add check for empty --file argument before running run-tests.sh during simpletest reviews [#1233248]

We've seen a couple error scenarios where no tests are actually run for a branch test or patch ... occasionally, this is due to run-tests.sh not being passed any tests via the --file argument. See the related PIFT issue #1156976: 0 tests passing is not green.

We should detect this condition and return an appropriate response instead of allowing a zero-test test to run and return green.

Comment	File	Size	Author
#13	pifr.empty_file_argument_check-1233248-13.patch	2.02 KB	jthorson
#11	pifr.empty_file_argument_check-1233248-11.patch	2 KB	jthorson
#10	pifr.empty_file_argument_check-1233248-10.patch	1.61 KB	jthorson
#8	pifr.empty_file_argument_check-1233248-8.patch	1.19 KB	jthorson
#2	pifr.add-no-valid-tests-check-1233248-2.patch	1.1 KB	jthorson
#1	pifr.add-no-valid-tests-check-1233248-1.patch	740 bytes	jthorson

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jthorson CreditAttribution: jthorson commented 29 July 2011 at 05:20

Status:

Active

» Needs review

File	Size
pifr.add-no-valid-tests-check-1233248-1.patch	740 bytes

Patch attached.

Comment #2

jthorson CreditAttribution: jthorson commented 29 July 2011 at 05:39

File	Size
pifr.add-no-valid-tests-check-1233248-2.patch	1.1 KB

Well ... that didn't work. New approach.

Comment #3

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 15:14

Comment #4

jthorson CreditAttribution: jthorson commented 1 August 2011 at 08:55

Merged this patch into the one posted at #1126112-11: Full tests of Drupal being done for some contrib patches, as they would otherwise conflict.

Comment #5

jthorson CreditAttribution: jthorson commented 2 August 2011 at 02:28

Status:

Needs review

» Fixed

Fix committed in referenced patch.

Comment #6

16 August 2011 at 02:32

Status:

Fixed

» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Comment #7

jthorson CreditAttribution: jthorson commented 30 September 2011 at 17:41

Status:

Closed (fixed)

» Active

Patch reversed, as the 'fail' condition prevents the ability to leverage testbots to ensure a patch can be applied against a project with no tests.

Comment #8

jthorson CreditAttribution: jthorson commented 20 November 2011 at 18:29

Status:

Active

» Needs review

File	Size
pifr.empty_file_argument_check-1233248-8.patch	1.19 KB

New approach, which should catch 'No tests found' due to testbot errors, but will not execute run_tests.sh if no arguments are found for the '--file ' parameter.

Comment #9

jthorson CreditAttribution: jthorson commented 20 November 2011 at 18:36

Status:

Needs review

» Needs work

After more thought, this won't actually catch the 'no tests found' error ... but is still a step in the right direction.

Comment #10

jthorson CreditAttribution: jthorson commented 20 November 2011 at 19:09

File	Size
pifr.empty_file_argument_check-1233248-10.patch	1.61 KB

Posting patch here so I can validate on scratchtestbot ...

Comment #11

jthorson CreditAttribution: jthorson commented 20 November 2011 at 22:00

File	Size
pifr.empty_file_argument_check-1233248-11.patch	2 KB

There ... I think this does the trick.

This patch takes the following approach:
i) Check for empty arguments on the "--file" parameter, and skip run_tests.sh if there are no tests to run
ii) Scan the review output for "PHP Fatal error", and fail the test if one is found ... thus fatal errors which interrupt the review process do not result in 'green' results.

This check could possibly be extended to include PHP Warning ... but I think the 'Fatal error' version this should catch 90% of our problem cases now.

Comment #12

jthorson CreditAttribution: jthorson commented 20 November 2011 at 22:13

Status:

Needs work

» Needs review

Looks promising ... tests result can be found at:

zero tests passed: http://rfay.redesign.devdrupal.org/node/1073590/testing-status
PHP Fatal error: http://rfay.redesign.devdrupal.org/node/19304/testing-status (6.x-1.x)
With actual tests passed: http://rfay.redesign.devdrupal.org/node/19304/testing-status (7.x-2.x)

Comment #13

jthorson CreditAttribution: jthorson commented 12 January 2012 at 06:35

File	Size
pifr.empty_file_argument_check-1233248-13.patch	2.02 KB

Rerolled patch.

Comment #14

jthorson CreditAttribution: jthorson commented 13 January 2012 at 02:16

Status:

Needs review

» Fixed

Committed to 6.x-2.x (264fb4f).

Couldn't find a patch which contained a FATAL but didn't break the testbot, so that component of the patch is unproven. Will need to watch for this scenario after deployment.