Page MenuHomePhabricator

Usages of unittest.expectedFailure and allowed_failure 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
  • page_tests.py
  • pagegenerators_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_tests.py
  • upload_tests.py

bold means bug tasks are created and marked as blocker

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenNone
Resolvedmatej_suchanek
OpenNone
OpenNone
ResolvedXqt
ResolvedDalba
Resolvedmatej_suchanek
ResolvedXqt
ResolvedMpaa
ResolvedXqt
ResolvedXqt
OpenNone
OpenNone
OpenNone
ResolvedXqt
OpenNone
ResolvedAbdealiJK
ResolvedXqt
OpenMagul
OpenXqt
ResolvedMpaa
OpenNone
ResolvedXqt
ResolvedDalba
OpenXqt

Event Timeline

Xqt created this task.Mar 9 2016, 5:16 PM
Restricted Application added subscribers: pywikibot-bugs-list, Aklapper. · View Herald TranscriptMar 9 2016, 5:16 PM
Xqt renamed this task from unittest.exprectedFailure should be solved to unittest.expectedFailure should be solved.Mar 9 2016, 5:17 PM
Xqt triaged this task as Low priority.Mar 18 2016, 2:48 PM
jayvdb updated the task description. (Show Details)Mar 19 2016, 3:31 AM

You should find explanations for most of them, which can be used to make this task more actionable.

jayvdb renamed this task from unittest.expectedFailure should be solved to Usages of unittest.expectedFailure should be solved.Mar 19 2016, 9:38 AM

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

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

Xqt updated the task description. (Show Details)Mar 25 2016, 1:07 PM
Xqt updated the task description. (Show Details)Mar 25 2016, 1:58 PM
Xqt updated the task description. (Show Details)Mar 25 2016, 6:26 PM
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

Xqt updated the task description. (Show Details)Mar 26 2016, 1:06 PM

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.

Mpaa added a comment.EditedMar 26 2016, 4:33 PM

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.

Xqt added a comment.Mar 27 2016, 10:15 AM

@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 updated the task description. (Show Details)Apr 21 2016, 1:19 PM
Xqt updated the task description. (Show Details)May 8 2016, 1:07 PM
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