Skip to content

Do not ignore pytest plugins nodes (like pep8) in tests discovery and results #11824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Do not ignore pytest plugins nodes (like pep8) in tests discovery and results #11824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcaa584
Merge pull request #1 from microsoft/master
zztalker Nov 6, 2019
147ff15
Merge pull request #2 from microsoft/master
zztalker Nov 19, 2019
e63fb51
Merge pull request #3 from microsoft/master
zztalker May 13, 2020
a5532c5
Do not ignore pytest plugins nodes in test discovering and results
zztalker May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/8425.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Do not ignore pytest plugins tests, like pep8. (thanks [Pavel Zaikin](https://github.com/zztalker/))
24 changes: 19 additions & 5 deletions pythonFiles/testing_tools/adapter/pytest/_pytest_item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,12 +155,20 @@ def parse_item(
"""
# _debug_item(item, showsummary=True)
kind, _ = _get_item_kind(item)
# Skip plugin generated tests
# Skip unexpected kind tests
if kind is None:
return None, None
(nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id(
item.nodeid, kind
)

if kind == "plugin_node":
(nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id(
item.nodeid + "::" + item.location[2], kind
Copy link

Choose a reason for hiding this comment

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

Suggested change
item.nodeid + "::" + item.location[2], kind
item.nodeid + "::" + item.location[2],
kind

Copy link

Choose a reason for hiding this comment

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

It isn't obvious why we're doing this, so a comment would be helpful here.

)

else:

(nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id(
Comment on lines +165 to +169
Copy link

Choose a reason for hiding this comment

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

Suggested change
)
else:
(nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id(
)
else:
(nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id(

item.nodeid, kind
)
# Note: testfunc does not necessarily match item.function.__name__.
# This can result from importing a test function from another module.

Expand Down Expand Up @@ -394,7 +402,7 @@ def _parse_node_id(
parents.append(node)
funcid, funcname, _ = node
parameterized = testid[len(funcid) :]
elif kind == "function":
elif kind == "function" or kind == "plugin_node":
funcname = name
else:
raise should_never_reach_here(
Expand Down Expand Up @@ -536,6 +544,12 @@ def _get_item_kind(item):
elif isinstance(item, pytest.Function):
# We *could* be more specific, e.g. "method", "subtest".
return "function", False
elif isinstance(item, _pytest.nodes.Item) and isinstance(item, _pytest.nodes.File):
Copy link

Choose a reason for hiding this comment

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

The condition here is not an obvious one. So it would be helpful to have a comment here with an explanation of how we identified the item as a plugin-generated node, along with information about plugin nodes in general. At the least a link to the relevant pytest docs would be good.

(FWIW, the same applies to the other kinds in this function. However, don't feel like you need to do that in this PR.)

# item._nodeid = item.nodeid + "::" + item.location[2]
item.name = item.name.replace(PATH_SEP, ".")
if item.name[-3:] == ".py":
item.name = item.name[:-3]
Comment on lines +548 to +551
Copy link

Choose a reason for hiding this comment

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

Let's not modify the item in this function. It should not have side effects. Consider adding a helper function that we can call up in parse_item():

def _fix_plugin_node(item):
    # item._nodeid = item.nodeid + "::" + item.location[2]
    item.name = item.name.replace(PATH_SEP, ".")
    if item.name[-3:] == ".py":
        item.name = item.name[:-3]

You could simplify calling code if you switch on the kind:

def _normalize_node(kind, item):
    if kind == "plugin_node":
        # item._nodeid = item.nodeid + "::" + item.location[2]
        item.name = item.name.replace(PATH_SEP, ".")
        if item.name[-3:] == ".py":
            item.name = item.name[:-3]

return "plugin_node", False
else:
return None, False

Expand Down
36 changes: 24 additions & 12 deletions src/client/testing/common/xUnitParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,18 +115,30 @@ function updateTests(tests: Tests, testSuiteResult: TestSuiteResult) {
updateResultInfo(testFunc, testcase);
updateResultStatus(testFunc, testcase);
} else {
// Possible we're dealing with nosetests, where the file name isn't returned to us
// When dealing with nose tests
// It is possible to have a test file named x in two separate test sub directories and have same functions/classes
// And unforutnately xunit log doesn't ouput the filename

// result = tests.testFunctions.find(fn => fn.testFunction.name === testcase.$.name &&
// fn.parentTestSuite && fn.parentTestSuite.name === testcase.$.classname);

// Look for failed file test
const fileTest = testcase.$.file && tests.testFiles.find((file) => file.nameToRun === testcase.$.file);
if (fileTest && testcase.error) {
updateResultStatus(fileTest, testcase);
// For pytest plugins like PEP8 it generate node for whole file, and classname == '' in it
Copy link

Choose a reason for hiding this comment

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

A slight adjustment, plus adding a docs URL to provide more explanation:

Suggested change
// For pytest plugins like PEP8 it generate node for whole file, and classname == '' in it
// For pytest plugins like PEP8 it generates a node for the whole file,
// with classname set to "".
// See: https://docs.pytest.org/...

if (testcase.$.classname === '') {
Copy link

Choose a reason for hiding this comment

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

How confident can we be that this condition is accurate enough? Are there plugin nodes that won't match this? Are there unsupported nodes that will match incorrectly? (Is this specific to the PEP8 plugin or to all plugins?) False positives here can result in problems for the extension, particularly due to how the test adapter currently works (relative to stdout).

Copy link

Choose a reason for hiding this comment

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

Let's keep the meaning of the data coming from the adapter script more consistent, such that this special case is unnecessary. That would mean updating the test adapter (in pythonFiles/testing_tools/adapter/pytest/_pytest_item.py) to change testcase.$.classname to the appropriate value in the kind == "plugin_node" case (rather than special-casing here in the extension code).

That would mean this file doesn't need to be changed at all.

const testFuncForPlugin = findTestFunction(tests.testFunctions, testcase.$.name, testcase.$.name);
if (testFuncForPlugin) {
if (testcase.$.line === '-1') {
testcase.$.line = '1';
}
Comment on lines +122 to +124
Copy link

Choose a reason for hiding this comment

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

Modification of the data should happen in the adapter script (i.e. pythonFiles/testing_tools/adapter/pytest/_pytest_item.py) rather than in the extension code. Please move this there.

updateResultInfo(testFuncForPlugin, testcase);
updateResultStatus(testFuncForPlugin, testcase);
}
} else {
// Possible we're dealing with nosetests, where the file name isn't returned to us
// When dealing with nose tests
// It is possible to have a test file named x in two separate test sub directories and have same functions/classes
// And unforutnately xunit log doesn't ouput the filename

// result = tests.testFunctions.find(fn => fn.testFunction.name === testcase.$.name &&
// fn.parentTestSuite && fn.parentTestSuite.name === testcase.$.classname);

// Look for failed file test
const fileTest = testcase.$.file && tests.testFiles.find((file) => file.nameToRun === testcase.$.file);
if (fileTest && testcase.error) {
updateResultStatus(fileTest, testcase);
}
}
}
});
Expand Down