Page MenuHomePhabricator

Usages of unittest.expectedFailure should be solved (goal)
Open, LowPublic

Description

There are a lot of unittest.expectedFailure decorators. There should be a good reason to keep it. Otherwise the problem should be solved.

I found some inside

  • aspects.py
  • api-tests.py
  • cosmetic_changes_tests.py
  • imagecopy_tests.py (archived)
  • isbn_tests.py (archived)
  • page_tests.py
  • pagegenerators_tests.py
  • pwb_tests.py
  • reflinks_tests.py
  • script_tests.py
  • site_detect_tests.py
  • site_tests.py
  • template_bot_tests.py
  • tests_tests.py
  • textlib_tests.py <-- some tests look wrong to me
  • tools_ip_tests.py
  • tools_tests.py
  • upload_tests.py
  • utils.py

bold means bug tasks are created and marked as blocker

Related Objects

StatusSubtypeAssignedTask
OpenNone
OpenNone
OpenNone
Resolvedmatej_suchanek
OpenNone
DeclinedNone
ResolvedXqt
ResolvedDalba
Resolvedmatej_suchanek
ResolvedXqt
ResolvedMpaa
ResolvedXqt
ResolvedXqt
OpenNone
OpenNone
OpenNone
ResolvedXqt
DeclinedNone
ResolvedAbdealiJK
ResolvedXqt
OpenNone
ResolvedXqt
ResolvedMpaa
ResolvedDalba
ResolvedXqt
ResolvedXqt
ResolvedXqt
ResolvedXqt
OpenNone
ResolvedXqt
OpenNone
DeclinedNone
ResolvedXqt
ResolvedJJMC89
ResolvedNone
ResolvedEBernhardson
ResolvedXqt
ResolvedBUG REPORTXqt

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 278456 had a related patch set uploaded (by Mpaa):
Solve usage of expectedFailure for tools_tests.py

https://gerrit.wikimedia.org/r/278456

Mpaa added a subscriber: Mpaa.

Change 278456 had a related patch set uploaded (by Mpaa):
Solve usage of expectedFailure for tools_tests.py

https://gerrit.wikimedia.org/r/278456

This is now tracked in T130985

On https://gerrit.wikimedia.org/r/#/c/278456/@xqt said

expectedFailure does not what is sounds. It also passes for unexpected failures and you cannot decide wether
the failure is expected or due to an unknown reason and the test is finally worthless.

@expectedFailure should only be used if the failure is 100% expected, and typically the failure is expected on the last line, and the test method is written so that other tests will fail if some earlier part of the test sequence has a problem. The test will become an "unexpected success" if the test passes.

(And we can configure "unexpected success" to be a treated as an error, but that only helps if someone is regularly looking for & fixing build breakages.)

Every test should be expressed in a form that it may fail in expected manner e.g. checking for wrong
results or for raised exceptions etc.

If the test is written so that it is the last line that fails (see above) the following are functionally equivalent

@expectedFailure
def test_foo(self):
     1 / 0

def test_foo2(self):
     with self.assertRaises(Exception):
         1 / 0

Except that the former has more semantic information, and code checkers explicitly fail self.assertRaises(Exception) as being too broad.

Of course we can work around that by using self.assertRaises(ZeroDivisionError): , but that means our tests are then explicitly linked to the underlying implementation of why it fails, which often isnt desirable because we dont have any control over that. expectedFailure is used to prove something doesnt work, irrespective of why, and often it exists to help explain a # TODO item by showing what 'should' work.

I would be very interesting to see any other project that has banned use of @expectedFailure.

I do not think this task is about banning, it is more about meaning of @expectedFailure.

In my view, @expectedFailure should not be used to check for a deliberate failure.
To test that in a given condition, a function is correctly raising an exception, the test should be written accordingly and pass.
@expectedFailure should be used when known bugs are present or similar (e.g. # TODO), in order to temporarily force the test case to pass and not to have the whole build marked as failed.
And when the issue is solved, it is probably better to remove it.

@jayvdb: expectedFailure decorator and assertRaises(Exception) as you mentioned above are not equivalent. expectedFailure always passes tests; it counts either 'expected failures' or 'unexpected success'. Whereas assertRaises(Exception) passes test if an exception occurred otherwise it fails the tests with 'Exception not raised'.

Xqt renamed this task from Usages of unittest.expectedFailure should be solved to Usages of unittest.expectedFailure and allowed_failure should be solved.Sep 11 2016, 6:58 AM
Xqt updated the task description. (Show Details)
Dvorapa renamed this task from Usages of unittest.expectedFailure and allowed_failure should be solved to Usages of unittest.expectedFailure and allowed_failure should be solved (goal).May 29 2018, 11:19 AM
Xqt renamed this task from Usages of unittest.expectedFailure and allowed_failure should be solved (goal) to Usages of unittest.expectedFailure should be solved (goal).Jan 22 2020, 12:53 PM
Xqt updated the task description. (Show Details)
Xqt updated the task description. (Show Details)