Skip to content

Change the output format of the pytest adapter.#4732

Merged
ericsnowcurrently merged 25 commits intomicrosoft:masterfrom
ericsnowcurrently:adapter-all-items
Mar 18, 2019
Merged

Change the output format of the pytest adapter.#4732
ericsnowcurrently merged 25 commits intomicrosoft:masterfrom
ericsnowcurrently:adapter-all-items

Conversation

@ericsnowcurrently
Copy link
Copy Markdown

@ericsnowcurrently ericsnowcurrently commented Mar 12, 2019

(for #4035)

We are moving to a relatively flat format that captures parent "nodes", in addition to tests. The new format looks like this:

[{
    "rootid": ".",
    "root": "/x/y/z",
    "parents": [{
        "id": "./test_spam.py",
        "kind": "file",
        "name": "test_spam.py",
        "parentid": "."
    }, {
        "id": "./test_spam.py::SpamTests",
        "kind": "suite",
        "name": "SpamTests",
        "parentid": "./test_spam.py"
    },
    "tests" [{
        "id": "./test_spam.py::test_all",
        "name": "test_all",
        "source": "test_spam.py:11",
        "markers": ["skip", "expected-failure"],
        "parentid": "./test_spam.py"
    }, {
        "id": "./test_spam.py::SpamTests::test_spam1",
        "name": "test_spam1",
        "source": "test_spam.py:23",
        "markers": ["skip"],
        "parentid": "./test_spam.py::SpamTests"
    }]

This also fixes a couple of bugs that I found while working on the change.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • [ ] Has a news entry file (remember to thank yourself!)
  • [ ] Has sufficient logging.
  • [ ] Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • [ ] Test plan is updated as appropriate
  • [ ] package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.

@ericsnowcurrently ericsnowcurrently added the no-changelog No news entry required label Mar 12, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 13, 2019

Codecov Report

Merging #4732 into master will increase coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #4732    +/-   ##
=======================================
+ Coverage      77%     77%    +1%     
=======================================
  Files         449     449            
  Lines       21659   21769   +110     
  Branches     3552    3564    +12     
=======================================
+ Hits        16617   16709    +92     
- Misses       5038    5056    +18     
  Partials        4       4
Flag Coverage Δ
#Linux 66% <ø> (ø) ⬇️
#Windows 66% <ø> (ø) ⬇️
#macOS 66% <ø> (ø) ⬇️

Copy link
Copy Markdown

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't review the code, however here are the issues I've come across:

  • It doesn't work with nested test suites.

  • Names are wrong

    • For a test suite, we're not getting the name of the test suite
    • We need to ensure . are removed, and when returning names of suites, the actual name of the test suite (class) is returned (else this breaks existing functionality)
    • Similarly when returning functions (with parameters) the name must be the name of the function
  • Also, do we need a separation of parents and functions.

    • Seems unnecessary when
    • we have the attribute kind to identify the type
    • we have parentid that tells us whether has a parent or not...
    • I.e. a flatter structure would also work, I'd just add the kind=test to the items in tests
    • just a thought, no need to change if you disagree)
  • Markers are not returned (please can we remove this)

    • Yes this may be a nice to have, however I'd go with YAGNI...
    • If its easy to add, then lets add it when we need it...
    • Its currently not working (I tested this with markers in suites and tests)
    • My suggestion is to add a separate issue to track that enhancement.
    • Also the contract doesn't support for markers in suites, more reason for not separating tests and parents (i.e. one single list)
    • Finally the term Markers is a pytest term, this isn't the term used in unit test nor in nose, they use the term decorators.

See below:

  • Suite ./tests/test_one.py::TestOne_WithChildren::TestOne_WithChildren_Child
    is a child of ./tests/test_one.py::TestOne_WithChildren
            {
                "id": "./tests/test_one.py::TestOne_WithChildren",
                "kind": "suite",
                "name": "TestOne_WithChildren",
                "parentid": "./tests/test_one.py"
            },
            {
                "id": "./tests/test_one.py::TestOne_WithChildren::TestOne_WithChildren_Child",
                "kind": "suite",
                "name": "TestOne_WithChildren.TestOne_WithChildren_Child",
                "parentid": "./tests/test_one.py"
            },

Here's what I would have expected:

            {
                "id": "./tests/test_one.py::TestOne_WithChildren",
                "kind": "suite",
                "name": "TestOne_WithChildren",
                "parentid": "./tests/test_one.py"
            },
            {
                "id": "./tests/test_one.py::TestOne_WithChildren::TestOne_WithChildren_Child",
                "kind": "suite",
                "name": "TestOne_WithChildren.TestOne_WithChildren_Child",
                "parentid": "./tests/test_one.py::TestOne_WithChildren"
            },

I.e. here's a concrete sample

class TestOne_WithChildren():
    def test_adding(self):
        assert 1 == 1

    def test_dividing(self):
        assert 1 == 1


    class TestOne_WithChildren_Child():
        def test_adding_sub(self):
            assert 1 == num

        def test_dividing_sub(self):
            assert 4/2 == 2

Please add tests to cover this scenario as well.

@ericsnowcurrently
Copy link
Copy Markdown
Author

Regarding nested suites, I'm not sure I see any problem. Here's an example of what this PR provides:

$ cat /tmp/tests/test_spam.py
class TestSpam:
    class TestEggs:
        def test_it(self):
            pass
$ python3.7 pythonFiles/testing_tools/run_adapter.py discover pytest /tmp/tests --verbose
[
    {
        "parents": [
            {
                "id": "./test_spam.py",
                "kind": "file",
                "name": "test_spam.py",
                "parentid": "."
            },
            {
                "id": "./test_spam.py::TestSpam",
                "kind": "suite",
                "name": "TestSpam",
                "parentid": "./test_spam.py"
            },
            {
                "id": "./test_spam.py::TestSpam::TestEggs",
                "kind": "suite",
                "name": "TestEggs",
                "parentid": "./test_spam.py::TestSpam"
            }
        ],
        "root": "/tmp/tests",
        "rootid": ".",
        "tests": [
            {
                "id": "./test_spam.py::TestSpam::TestEggs::test_it",
                "markers": [],
                "name": "test_it",
                "parentid": "./test_spam.py::TestSpam::TestEggs",
                "source": "./test_spam.py:4"
            }
        ]
    }
]

@DonJayamanne
Copy link
Copy Markdown

@pytest.mark.webtest
class TestOne_WithChildren():
    def test_adding(self):
        # time.sleep(5)
        # num = wow.returnNumber()
        # assert 1 == num
        pass
    @pytest.mark.webtest
    def test_dividing(self):
        # time.sleep(5)c
        assert 1 == 1


    class TestOne_WithChildren_Child():
        def test_adding_sub(self):
            # time.sleep(5)
            # num = wow.returnNumber()
            # assert 1 == num
            assert True

        def test_dividing_sub(self):
            # time.sleep(5)c
            assert 4/2 == 2

@DonJayamanne
Copy link
Copy Markdown

It doesn't work with pytest repo.
Here's the command line:

python /Users/donjayamanne/.vscode-insiders/extensions/pythonVSCode/pythonFiles/testing_tools/run_adapter.py discover pytest

Here's the error response

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/_pytest/main.py", line 210, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/_pytest/main.py", line 249, in _main
INTERNALERROR>     config.hook.pytest_collection(session=session)
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/hooks.py", line 289, in __call__
INTERNALERROR>     return self._hookexec(self, self.get_hookimpls(), kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/manager.py", line 68, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/manager.py", line 62, in <lambda>
INTERNALERROR>     firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/callers.py", line 208, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/callers.py", line 81, in get_result
INTERNALERROR>     _reraise(*ex)  # noqa
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/callers.py", line 187, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/_pytest/main.py", line 259, in pytest_collection
INTERNALERROR>     return session.perform_collect()
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/_pytest/main.py", line 492, in perform_collect
INTERNALERROR>     hook.pytest_collection_finish(session=self)
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/hooks.py", line 289, in __call__
INTERNALERROR>     return self._hookexec(self, self.get_hookimpls(), kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/manager.py", line 68, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/manager.py", line 62, in <lambda>
INTERNALERROR>     firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/callers.py", line 208, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/callers.py", line 81, in get_result
INTERNALERROR>     _reraise(*ex)  # noqa
INTERNALERROR>   File "/usr/local/lib/python2.7/site-packages/pluggy/callers.py", line 187, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/Users/donjayamanne/.vscode-insiders/extensions/pythonVSCode/pythonFiles/testing_tools/adapter/pytest.py", line 96, in pytest_collection_finish
INTERNALERROR>     test, suiteids = _parse_item(item, self.NORMCASE, self.PATHSEP)
INTERNALERROR>   File "/Users/donjayamanne/.vscode-insiders/extensions/pythonVSCode/pythonFiles/testing_tools/adapter/pytest.py", line 212, in _parse_item
INTERNALERROR>     raise NotImplementedError
INTERNALERROR> NotImplementedError
Traceback (most recent call last):
  File "/Users/donjayamanne/.vscode-insiders/extensions/pythonVSCode/pythonFiles/testing_tools/run_adapter.py", line 13, in <module>
    main(tool, cmd, subargs, toolargs)
  File "/Users/donjayamanne/.vscode-insiders/extensions/pythonVSCode/pythonFiles/testing_tools/adapter/__main__.py", line 81, in main
    parents, result = run(toolargs, **subargs)
  File "/Users/donjayamanne/.vscode-insiders/extensions/pythonVSCode/pythonFiles/testing_tools/adapter/pytest.py", line 31, in discover
    raise Exception('pytest discovery failed (exit code {})'.format(ec))
Exception: pytest discovery failed (exit code 3)

@DonJayamanne
Copy link
Copy Markdown

DonJayamanne commented Mar 14, 2019

@ericsnowcurrently Not sure whats wrong, but doesn't work now.

@d3r3kk d3r3kk closed this Mar 15, 2019
@d3r3kk d3r3kk reopened this Mar 15, 2019
@ericsnowcurrently ericsnowcurrently merged commit 3d5af66 into microsoft:master Mar 18, 2019
@ericsnowcurrently ericsnowcurrently deleted the adapter-all-items branch March 18, 2019 16:24
@lock lock Bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants