Page MenuHomePhabricator
Create Task
Maniphest T10282

Unsound test exit status creates false positives
Open, Needs TriagePublic
Actions

Description

I did read and take to heart your article on exit codes. I am however faced with a tricky situation in that the exit code used for an unsound test is 1 in ArcanistUnitWorkflow.php.

The issue that we are running into specifically is that we have a harbormaster build step that runs arc unit. In this step we rely on the exit status of arc to determine if it passed or failed. In practice this means that a status of 0 or 1 (SUCCESS or UNSOUND) means everything is "fine" and a status of anything else means it failed. however in doing some testing today I noticed that it's also possibly for arc to blow up before even getting to running the unit tests in which case all the unit tests get skipped and we have no way to distinguish this output or status code from that of an unsound test since a php fatal error exits with an exit code of one as well.

Here is an example:

Exception
2	Command failed with error #1!
3	COMMAND
4	'/var/drydock/workingcopy-3399/repo/super/secret/sauce' -c '/var/drydock/workingcopy-3399/repo/super/secret/sauce/something.yml' --manifest
5	
6	STDOUT
7	(empty)
8	
9	STDERR
10	Traceback (most recent call last):
11	  File "/var/drydock/workingcopy-3399/repo/super/secret/sauce", line 724, in <module>
12	    sys.exit(main())
13	  File "/var/drydock/workingcopy-3399/repo/super/secret/sauce/run_unit_tests", line 694, in main
14	    print json.dumps(generate_manifest(), indent=2)
15	  File "/var/drydock/workingcopy-3399/repo/super/secret/sauce/run_unit_tests", line 635, in generate_manifest
16	    for test in discoverTests():
17	  File "/var/drydock/workingcopy-3399/repo/super/secret/sauce/run_unit_tests", line 276, in discoverTests
18	    for filename in sorted(os.listdir(unitTestDir)):
19	OSError: [Errno 2] No such file or directory: '/var/drydock/workingcopy-3399/repo/super/secret/sauce/bin/tests'
20	
21	(Run with `--trace` for a full exception trace.)

It seems like in this case error codes are already being used to signal the status of the command and having unsound overlap with a catastrophic failure could be problematic.

Event Timeline

michaeljs1990 renamed this task from Unsound test exit status creates false positives to Unsound test exit status creates false negatives.Feb 5 2016, 6:31 AM
michaeljs1990 renamed this task from Unsound test exit status creates false negatives to Unsound test exit status creates false positives.
michaeljs1990 created this task.
michaeljs1990 added a subscriber: yelirekim.
michaeljs1990 updated the task description. (Show Details)Feb 5 2016, 6:35 AM
avivey added a subscriber: avivey.Feb 6 2016, 4:10 AM

I don't fully understand the issue description:

  • The "command failed with error #1" content - is that the output of arc unit? Or is the HM job invoking sauce directly?
  • Is the chief complaint that arc unit exits with 1 for both some cases of exceptions and some cases of successful runs (For some value of "success")?
  • What, in your scenario, is reading the exit code of arc unit?
michaeljs1990 added a comment.EditedFeb 6 2016, 4:51 AM
  • @avivey we have our own library that is loaded in when arc runs. I had an out of date version of our library which caused this to throw an error.
  • The chief complaint is that arc unit for unsound tests exits with a status of one since it's indistinguishable from arc completely failing.
  • A harbormaster build step is reading the exit status to determine if the test was a real failure or just the result of an unsound test not passing.
michaeljs1990 added a comment.Mar 8 2016, 1:49 AM

Sorry to be a bother on this again. Today we almost got bitten bad by this again since the unsound test returns an exit code of 1 and the failure was actually a class not being loaded because the user had not upgraded arc on the machine.

avivey added a comment.Mar 8 2016, 2:05 AM

IIRC, the "right" thing to do here (According to that article), is to exit with 0 in all non-exception cases - i.e., even for failing / unsound / broken tests - basically, remove https://secure.phabricator.com/diffusion/ARC/browse/master/scripts/arcanist.php;ccbaee585e1a350d1ff970c30481079dc8471a7c$399 and expect exceptions whenever something is wrong.

michaeljs1990 added a comment.Mar 8 2016, 6:03 AM

I have been trying very hard to run arcanist / pharbicator /libphutil unforked until now so I would strongly prefer not to edit any of the source code. The second issue is that this is being run as an external command since it's a drydock build step.

michaeljs1990 updated the task description. (Show Details)Mar 8 2016, 6:09 AM
michaeljs1990 updated the task description. (Show Details)Mar 8 2016, 4:12 PM
michaeljs1990 updated the task description. (Show Details)
Phabricator · Built by Phacility