[tests] Add Subprocess TestCase by Kyle-Verhoog · Pull Request #720 · DataDog/dd-trace-py

Kyle-Verhoog · 2018-11-19T11:08:48Z

Overview

This PR introduces a subprocess test case which can be extended to achieve the functionality of running test cases in a new python process.

The test cases can be run in pytest nose and unittest.

Example

Adding the following test case to the gevent test suite in a file test_patch.py:

@run_in_subprocess
class TestGeventPatch1(SubprocessTestCase):
    def test_patch_before_import_1(self):
        # assert False
        patch(gevent=True)
        import gevent
        assert True

    def test_patch_after_import_1(self):
        import gevent
        patch(gevent=True)
        assert True

class TestGeventPatch2(SubprocessTestCase):
    @run_in_subprocess
    def test_patch_before_import_2(self):
        patch(gevent=True)
        import gevent
        assert True

    def test_patch_after_import_2(self):
        patch(gevent=True)

Running with pytest:

~/d/dd-trace-py ❯❯❯ pytest tests/contrib/gevent/test_patch.py                                                                            
============================================================ test session starts =============================================================
platform darwin -- Python 3.7.0, pytest-4.0.0, py-1.7.0, pluggy-0.8.0
rootdir: /Users/kyle.verhoog/dev/dd-trace-py, inifile:
collected 4 items                                                                                                                            

tests/contrib/gevent/test_patch.py ....                                                                                                [100%]

========================================================== 4 passed in 0.43 seconds ==========================================================

Running with nosetests (note that the reported number of tests is off, but the dots indicate the tests have run):

~/d/dd-trace-py ❯❯❯ nosetests tests/contrib/gevent/test_patch.py
....
----------------------------------------------------------------------
Ran 1 test in 0.360s

OK

Running with unittest:

~/d/dd-trace-py ❯❯❯ python -m unittest discover -t . tests/contrib/gevent/ 
............................
----------------------------------------------------------------------
Ran 20 tests in 1.210s

OK

- adds support for future patch_test.py - adds support for running tests in new interpreter instance - uses requests, gevent and celery as examples of usage in tox.ini

…propriately

brettlangdon

what happens if a we run a clean test without this runner?

I assume they will just fail?

Any way we can block them from executing unless our runner is used? Or show a warning or something?

(probably not actually necessary since them failing tells you you did something wrong anyways).

brettlangdon

We might be able to just create a subclass of unittest.TestCase

https://github.com/python/cpython/blob/c743a6ac360cfa1d9de7e6fad98b68fca39367d5/Lib/unittest/case.py#L580-L647

It might require a little finesse to get correct though.

class DDTestCase(unittest.TestCase):
    def run(self, result=None):
        # TODO: Setup stuff
     
        test_method = getattr(self, self._testMethod)
        if not is_cleantest(test_method):
            return super(self, DDTestCase).run(result=result) 

        if running_in_subprocess:
            # TODO: Maybe we can do `pickle.loads(sys.STDIN)` to get the `result` from the caller?
            super(self, DDTestCase).run(result=result)
            # TODO: `pickle.dumps(result)`
        else:
            # TODO: Run subprocess to re-run this specific `test_method`
            # TODO: `result = pickle.loads(process.stdout)` (if we do `pickle.loads(stdin)` above, otherwise we get the result and then update our existing `result` with the one from this subprocess)

If we do it this way then we can just do pytest tests/contrib/redis and do not need to change anything up there, and it means we can intermix "clean" tests with non-clean tests just by adding a decorator to the function.

While probably a little more complicated to get correct, this has less moving pieces, less things we need to think about when setting up a test, will be portable between test runners, etc.

(Make sure to not read this as a mandated change, just a different approach that we can have a conversation about).

brettlangdon · 2018-11-19T22:25:31Z

Also, while we are here, not 100% sold on clean/cleantest as the name for this, not sure it fully communicates what we want, especially to someone who looks at our codebase for the first time.

Even if we go for CleanTestRunner or something like that, we might want something more obvious for the decorator, like @run_in_subprocess.

Clarity will usually always out-weight succinctness.

Kyle-Verhoog · 2018-11-20T10:44:47Z

what happens if a we run a clean test without this runner?

The clean tests should run normally in whatever runner runs them.

I assume they will just fail?

They shouldn't. A downside is, is that we don't have a way to route cleantests to the cleanrunner. We can't ensure that they will be invoked by the cleantest runner.

Any way we can block them from executing unless our runner is used? Or show a warning or something?

I'm not sure if we can make the tests aware of the runner they are executed from.

We might be able to just create a subclass of unittest.TestCase

This is an interesting idea. We lose the parallelism but gain compatibility with the other test frameworks, which I like. This approach also means each test case that we want to run clean will have to extend the base class and decorate the method (a slightly implicit constraint).

Also, while we are here, not 100% sold on clean/cleantest as the name for this, not sure it fully communicates what we want, especially to someone who looks at our codebase for the first time.

Agreed. I didn't spend too much time on the naming, and found that coming up with a concise name for this kind of behaviour is tricky. I'm completely open to naming changes as this was just a placeholder.

I'm going to explore a more elegant solution to routing tests to a runner which removes the brittleness of needing to specify test_patch.py. I'll explore the subclassing approach since I think this is a cleaner approach for running the test.

brettlangdon · 2018-11-20T12:43:31Z

All your replies here sound great.

Re: parallelism, leave that to the test runner. pytest supports running test cases in parallel, we just need to make sure our code is capable of running in parallel, e.g if they run in a subprocess then shouldn't be a problem, or if they don't all reply on the same redis server, etc etc.

Kyle-Verhoog · 2018-11-21T13:37:38Z

@brettlangdon I tried several different approaches with the ideas we discussed. Most failed with the serialization step as tracebacks are not serializable and hence we couldn't send back the required stack trace of the failed test case to the parent process.

The approach I've switched to is to let the test run normally and use the return code to indicate success/failure. The appropriate result API methods are then called based accordingly but with an artificial exception raised (since we can't get this info from the child). The test results from the child process are printed to stderr for debugging.

This from my limited testing works for pytest, nose and unittest.

brettlangdon

Some initial questions/thoughts, but looks good, I like it

- use environment variables for subprocess detection - remove comments references to 'parallel' - correct examples - remove hasattr check

brettlangdon

minor nits, otherwise good to go.

brettlangdon

this is awesome. thanks for this work!!!

Kyle-Verhoog added 2 commits November 19, 2018 11:31

[tests] add custom test runners

16fadb4

- adds support for future patch_test.py - adds support for running tests in new interpreter instance - uses requests, gevent and celery as examples of usage in tox.ini

[tests] documentation

7a49940

Kyle-Verhoog added enhancement dev/testing labels Nov 19, 2018

Kyle-Verhoog added this to the 0.17.0 milestone Nov 19, 2018

This was referenced Nov 19, 2018

[tests] Add patch mixin and base test case #721

Merged

[core] Prepare for module import hook usage #681

Closed

Kyle-Verhoog added 3 commits November 19, 2018 14:18

[tests] allow custom number of procs, clean-up sys.exit

f732439

[tests] fix __package__ for patch tests, handle non 0 return codes ap…

1aa2c5f

…propriately

[tests] handle cleantests properly

dc45c93

brettlangdon reviewed Nov 19, 2018

View reviewed changes

Comment thread tox.ini Outdated

brettlangdon reviewed Nov 19, 2018

View reviewed changes

Comment thread tests/cleantest.py Outdated

Comment thread tests/cleantest.py Outdated

Comment thread tests/cleantest.py Outdated

brettlangdon reviewed Nov 19, 2018

View reviewed changes

Comment thread tests/runner.py Outdated

Kyle-Verhoog added 3 commits November 20, 2018 15:25

[tests] replace runners and test suite with testcase approach

d006be6

[tests] remove serialization

bfed5a0

[tests] achieve compatibility with pytest, nosetests and unittest

c9267e9

Kyle-Verhoog changed the title ~~[tests] Add custom test runners~~ [tests] Add Subprocess TestCase Nov 21, 2018

brettlangdon modified the milestones: 0.17.0, 0.18.0 Nov 21, 2018

brettlangdon reviewed Nov 26, 2018

View reviewed changes

Comment thread tests/subprocesstest.py Outdated

Comment thread tests/subprocesstest.py Outdated

Comment thread tests/subprocesstest.py Outdated

Comment thread tests/subprocesstest.py Outdated

Comment thread tests/subprocesstest.py

[subprocesstest] address comments

7bee998

- use environment variables for subprocess detection - remove comments references to 'parallel' - correct examples - remove hasattr check

brettlangdon reviewed Nov 27, 2018

View reviewed changes

Comment thread tests/subprocesstest.py

Comment thread tests/subprocesstest.py Outdated

Comment thread tests/subprocesstest.py Outdated

Comment thread tests/subprocesstest.py Outdated

[subprocesstest] clean-up

4d46dda

brettlangdon approved these changes Nov 27, 2018

View reviewed changes

Comment thread tests/subprocesstest.py

Comment thread tests/subprocesstest.py

Kyle-Verhoog changed the base branch from master to 0.18-dev November 27, 2018 14:54

Merge branch '0.18-dev' into kyle-verhoog/cleantest

d87c475

Kyle-Verhoog merged commit 2d78bea into 0.18-dev Nov 27, 2018

Kyle-Verhoog deleted the kyle-verhoog/cleantest branch November 27, 2018 15:34

brettlangdon mentioned this pull request Dec 4, 2018

[core] vendor wrapt and six dependencies #755

Merged

Uh oh!

Conversation

Kyle-Verhoog commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Example

Uh oh!

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brettlangdon commented Nov 19, 2018

Uh oh!

Kyle-Verhoog commented Nov 20, 2018

Uh oh!

brettlangdon commented Nov 20, 2018

Uh oh!

Kyle-Verhoog commented Nov 21, 2018

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brettlangdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Kyle-Verhoog commented Nov 19, 2018 •

edited

Loading