Do not ignore pytest plugins nodes (like pep8) in tests discovery and results #11824
Conversation
Update upstream
Sync to upstream
Sync to upstream
Codecov Report
@@ Coverage Diff @@
## master #11824 +/- ##
==========================================
- Coverage 60.45% 60.42% -0.04%
==========================================
Files 631 631
Lines 34153 34163 +10
Branches 4797 4801 +4
==========================================
- Hits 20648 20642 -6
- Misses 12513 12518 +5
- Partials 992 1003 +11
Continue to review full report at Codecov.
|
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Thanks for the PR, @zztalker. The overall approach looks okay, but I don't have much context on plugin-generated nodes to be sure.
| # item._nodeid = item.nodeid + "::" + item.location[2] | ||
| item.name = item.name.replace(PATH_SEP, ".") | ||
| if item.name[-3:] == ".py": | ||
| item.name = item.name[:-3] |
There was a problem hiding this comment.
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]| @@ -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): | |||
There was a problem hiding this comment.
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.)
| ) | ||
|
|
||
| else: | ||
|
|
||
| (nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id( |
There was a problem hiding this comment.
| ) | |
| else: | |
| (nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id( | |
| ) | |
| else: | |
| (nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id( |
|
|
||
| if kind == "plugin_node": | ||
| (nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id( | ||
| item.nodeid + "::" + item.location[2], kind |
There was a problem hiding this comment.
| item.nodeid + "::" + item.location[2], kind | |
| item.nodeid + "::" + item.location[2], | |
| kind |
|
|
||
| if kind == "plugin_node": | ||
| (nodeid, parents, fileid, testfunc, parameterized) = _parse_node_id( | ||
| item.nodeid + "::" + item.location[2], kind |
There was a problem hiding this comment.
It isn't obvious why we're doing this, so a comment would be helpful here.
| 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 |
There was a problem hiding this comment.
A slight adjustment, plus adding a docs URL to provide more explanation:
| // 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 (fileTest && testcase.error) { | ||
| updateResultStatus(fileTest, testcase); | ||
| // For pytest plugins like PEP8 it generate node for whole file, and classname == '' in it | ||
| if (testcase.$.classname === '') { |
There was a problem hiding this comment.
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).
| if (testcase.$.line === '-1') { | ||
| testcase.$.line = '1'; | ||
| } |
There was a problem hiding this comment.
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.
| if (fileTest && testcase.error) { | ||
| updateResultStatus(fileTest, testcase); | ||
| // For pytest plugins like PEP8 it generate node for whole file, and classname == '' in it | ||
| if (testcase.$.classname === '') { |
There was a problem hiding this comment.
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.
|
@zztalker, oh, and please make sure to add tests sooner rather than later. They will help to nail down the target behavior. |
Codecov Report
@@ Coverage Diff @@
## main #11824 +/- ##
==========================================
- Coverage 60.45% 59.88% -0.58%
==========================================
Files 631 682 +51
Lines 34153 37942 +3789
Branches 4797 5465 +668
==========================================
+ Hits 20648 22721 +2073
- Misses 12513 14044 +1531
- Partials 992 1177 +185
Continue to review full report at Codecov.
|
|
Closing as out of date. |
POC. Gather plugins nodes (for pytest-pep8 it is (Item, File)) and change nodeid and name of a function to adopt it for current tests processing workflow.
For #8425
Has sufficient logging.Has telemetry for enhancements.Unit tests & system/integration tests are added/updated.Test plan is updated as appropriate.package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).The wiki is updated with any design decisions/details.