SatelliteQE / robottelo

Robottelo is a test suite that exercises The Foreman.
GNU General Public License v3.0
61 stars 112 forks source link

Drop DDT in favor of subTest (available to python 2 on the unittest2) #2769

Closed elyezer closed 8 years ago

elyezer commented 9 years ago

@ddt decorator current usage:

Snip! See comment below.

Ichimonji10 commented 9 years ago

Why are we dropping DDT in favor of subTest? We're doing so because the former approach generates test data when tests are defined, whereas the latter approach generates test data at run-time.

Need a concrete example? Here's a test that makes use of DDT:

#!/usr/bin/env python
from ddt import ddt, data
from unittest import TestCase

@ddt
class NumbersTest(TestCase):

    @data(*range(6))
    def test_even(self, num):
        self.assertEqual(num % 2, 0)

Here's an equivalent example that uses subTest:

#!/usr/bin/env python
from unittest2 import TestCase  # or `from unittest` if on Python 3.4+

class NumbersTest(TestCase):
    def test_even(self):
        for i in range(6):
            with self.subTest(i):
                self.assertEqual(i % 2, 0)

Example output with DDT:

$ python -m unittest test2.py
.F.F.F
======================================================================
FAIL: test_even_2_1 (test2.NumbersTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ichimonji10/env3/lib/python3.4/site-packages/ddt.py", line 146, in wrapper
    return func(self, *args, **kwargs)
  File "/home/ichimonji10/tmp/test2.py", line 11, in test_even
    self.assertEqual(num % 2, 0)
AssertionError: 1 != 0
…
----------------------------------------------------------------------
Ran 6 tests in 0.001s

FAILED (failures=3)

Example output with subTest:

$ python -m unittest2 test.py

======================================================================
FAIL: test_even (test.NumbersTest) [1]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test.py", line 9, in test_even
    self.assertEqual(i % 2, 0)
AssertionError: 1 != 0
…
----------------------------------------------------------------------
Ran 1 test in 0.001s

FAILED (failures=3)

Regardless of whether the tests are run with DDT or subTest, three failures are reported. This is nice for reporting and tracking purposes. It indicates that test results should integrate well with our existing Jenkins set-up either way. That said:


It's pretty easy to switch from unittest to unittest2. All you need to do is list unittest2 as a dependency in setup.py and do some replacements:

for file in $(grep -RI --files-with-matches unittest .); do
    sed -i 's/unittest/unittest2/' "$file";
done

Unfortunately, that's only half the story, and the second half of the story is problematic. Once we've switched to unittest2, we can start dropping DDT and making use of subTest instead. The problem here is that none of the well-known test runners deals nicely with subTest. nosetests, nose2 and py.test all fail an entire test as soon as a single assertion inside of that test fails, even if the assertion is wrapped in a subTest context manager. That completely destroys the point of using subTest!

We can try using the vanilla test runner that ships with unittest2, like so:

python -m unittest2 …

But unittest2 doesn't have any native ability to generate JUnit style XML output. It's possible to address this issue by using unittest2 in combination with a separate test runner. In particular, you can use the unittest-xml-reporting test runner. Unfortunately:

That's a blocker.


My goals here are to:

And I'm not sure how to do this right now.

Ichimonji10 commented 9 years ago

I think I'll run throught the training for our new TCMS and see if that provides any guidance on where to go from here. Also, it's worth noting that unittest-xml-reporting is a BSD-licensed open source project, so if it becomes clear that using unittest and unittest2 is ideal, then fixing xmlrunner/unittest-xml-reporting#85 may be appropriate.

elyezer commented 9 years ago

In the meantime, running with nose will have only one test result reported and will fix the random name issue. We can make providing junit based report as next step.

Ichimonji10 commented 9 years ago

In the meantime, running with nose will have only one test result reported and will fix the random name issue. We can make providing junit based report as next step.

That's 100% true. Dropping DDT will fix the random name issue, and we can improve junit test reports as time goes on.

Ichimonji10 commented 8 years ago

As a reminder, we're dropping DDT from Robottelo primarily so that we can have stable test names. If test names are stable, then:

Also, we'll be able to drop some DDT-specific hacks from Robottelo.

I believe the best way to accomplish this is to make use of the subTest context manager. subTest was introduced to the standard library's unittest in Python 3.4, and it has been backported to earlier versions of Python via the unittest2 package.

For instructions on using subTest, read Distinguishing test iterations using subtests. For a usage example from our own test suite, see tests.foreman.api.test_contentview (especially after #2794 is merged). Take special note of the fact that subTest lets you re-use test resources, which lets tests run much more quickly.

unittest2 has been added to Robottelo's list of requirements already. (See setup.py.) Several of the test modules in tests.foreman.api have been converted over, and the conversion has gone well. I take this as a proof that moving away from DDT is feasible. However, I have not converted all of the modules in tests.foreman.api, and I have not even started on the CLI or API tests, and our documentation is also quite out-of-date.

Moving away from DDT means that (nearly?) every test module will need to be visited and updated. Along with this will come the temptation to make other changes. Move a parentheses here, push some brackets there, and so on. I'm in favor of improving Robottelo in ways both large and small, but making these sorts of changes while moving away from DDT makes it more likely that we'll introduce unintentional errors, and it also will obscure the intent of the pull requests. Thus, I recommend tackling #2795.

Ichimonji10 commented 8 years ago

Files to be updated:

p.s.

git grep --files-with-matches ddt | awk '{print "* [ ] `" $0 "`"}' | xclip -sel clip
elyezer commented 8 years ago

Much better, thanks for updating.

sthirugn commented 8 years ago

As of now, there are almost 73 files remaining. As part of ddt removal few performance improvements are also being done. Going at the rate of 3 per day this would take 24 business days (fixing review comments/ retest efforts extra)

lpramuk commented 8 years ago

Q: There is tearDown in TestImport class, with ddt it was called everytime (as subtests are rendered as separate tests) With subtest it is called only once, i.e. not between subtests :(

Should we call it explicitly within for loop? Or remove it (for oneliners incorporate into loop) Or rename it (for moreliners) and call explicitly within loop, coz without rename tearDown would be called twice (in the last subtest and after whole test)

Ichimonji10 commented 8 years ago

I assume you're talking about tests.foreman.cli.test_import.TestImport?

Should we call it explicitly within for loop?

No. setUpModule, setUpClass,setUp, and the correspondingtearDown*` methods all have specific semantics. Don't mess with them. That's a recipe for introducing subtle bugs, and it'll confuse other devs who have to read the code.

lpramuk commented 8 years ago

Ok. That's why I asked: - they're special + setUp and tearDown with ddt removed works differently now I tended to do it as you said. I go for "Rename tearDown to cleanUp and call explicitly within for loop"

sthirugn commented 8 years ago

@lpramuk your last comment sounds good to me.

sthirugn commented 8 years ago

All items are addressed and https://github.com/SatelliteQE/robottelo/issues/2969, https://github.com/SatelliteQE/robottelo/issues/2968 are created to track related issues.

Ichimonji10 commented 8 years ago

:100: