Page MenuHomePhabricator
Create Task
Maniphest T4570

ArcanistLinterTestCase::executeTestsInDirectory() should fail when it finds 0 tests.
Closed, ResolvedPublic
Actions

Description

D7910 changed the behavior of these tests to only test files which end in .lint-test. Not being aware of this, after updating Arcanist all of our lint tests continued to pass however they were actually no longer testing anything. As future changes were made to linters, the tests started to rot.

Ideally, after pulling this update, this change should have been immediately apparent by having these tests fail with a clear error message like

No tests found in $dir with suffix .lint-test. Ensure $dir is not empty and test files end with .lint-test

Would allow the breakage to be quickly spotted and can follow through with renaming some files. This change will also ensure the test will not pass if the directory name is misspelled.

Revisions and Commits

rPHU libphutil
D8435rPHU01e849c9bafc Add assertions to every test in libphutil/
rARC Arcanist
D8437rARCc6b1f3f070f6 Fail Arcanist tests when they make zero assertions
D8434rARC22f62079205f Fail lint tests if no tests are found in the test directory
rP Phabricator
D8436rP0488ce73c4b2 Add assertions to no-assertions tests in phabricator/

Event Timeline

leebyron created this task.Mar 7 2014, 4:49 AM
leebyron assigned this task to epriestley.
leebyron raised the priority of this task from to Needs Triage.
leebyron updated the task description. (Show Details)
leebyron added a project: Arcanist.
leebyron added subscribers: leebyron, joshuaspence, beng.
epriestley added a comment.Mar 7 2014, 2:01 PM

Yeah, this is silly.

In the general case, we should probably fail tests which make no assertions (we likely have a few tests which pass by not throwing exceptions, but we could add trivial assertions in these cases).

epriestley edited this Maniphest Task.Mar 7 2014, 2:07 PM
epriestley edited this Maniphest Task.Mar 7 2014, 2:16 PM
epriestley edited this Maniphest Task.Mar 7 2014, 2:20 PM
epriestley edited this Maniphest Task.Mar 7 2014, 2:23 PM
epriestley triaged this task as Normal priority.Mar 7 2014, 3:17 PM
epriestley edited this Maniphest Task.Mar 7 2014, 6:02 PM
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.
epriestley closed this task as Resolved.Mar 7 2014, 6:03 PM

Closed by commit rARCc6b1f3f070f6.

epriestley added a comment.Mar 7 2014, 6:03 PM

This should be fixed in HEAD. Thanks for the report, and let us know if you run into anything else.

leebyron added a subscriber: epriestley.Mar 7 2014, 7:23 PM

Thanks for the quick response @epriestley! I agree with you that the general case of failing a test when nothing was asserted is a good idea. I think it still may be a good idea for this particular case to have a clear error message, but at least now we're failing fast.

leebyron added a comment.Mar 7 2014, 7:30 PM

Ah I see you did that too :)

Phabricator · Built by Phacility