Fail Arcanist tests when they make zero assertions
c6b1f3f070f6
Actions

Description

Fail Arcanist tests when they make zero assertions

Summary:
Fixes T4570. When a test case doesn't make any assertions, fail it:

A tiny fraction of tests pass by not throwing. These tests can easily make a trivial assertion. It took about 5 minutes to fix them all (D8435, D8436).
In other cases, no assertions means a test construction problem, as with T4570. In these cases, failing loudly catches a severe error.
Fixes the no-assertion test cases in arcanist/
Makes the PHP 5.4 test pass for the moment, see discussion in T4334.

Test Plan: Ran arc unit --everything.

Reviewers: leebyron, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4570

Differential Revision: https://secure.phabricator.com/D8437

Details

Provenance

epriestley	Authored on
epriestley	Pushed on Mar 7 2014, 6:03 PM

Reviewer

btrahan

Differential Revision

D8437: Fail Arcanist tests when they make zero assertions

Parents

rARC22f62079205f: Fail lint tests if no tests are found in the test directory

Branches

Unknown

Tags

Unknown

Tasks

T4570: ArcanistLinterTestCase::executeTestsInDirectory() should fail when it finds 0 tests.

Event Timeline

leebyron added inline comments.Mar 7 2014, 7:24 PM

/src/unit/engine/__tests__/XUnitTestResultParserTestCase.php
41	If you end up with a 3rd one of these, `$this->assertReached()` might be a nice clarification of the pattern.

We had a handful in the other libraries (maybe 8 or 9 total) so we're over 3, but under, like, 10.

I agree with the notion in general, but I've been fairly hesitant to introduce new convenience assertX() functions which just wrap assertEqual(). I like that every assertion is assertEqual(), under the theory that makes tests easier to read (albeit slightly harder to write) since you don't have to look at which flavor of assertion is being invoked. I recall making mistakes in the past when stumbling over assertNotLessThanOrUnequalToUnlessNull() or similar at Facebook.

I think I'm probably in the wrong on this one and will eventually cave and add a bunch of assertXYZ() methods -- or, at least, an assertTrue() -- but am clinging to purity until then. (There's also a much stronger argument in my mind for assertReached() than other flavors which simply are convenience versions of assertEqual().)