Skip to content

Discovered tests may have problematic filenames. #6820

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.

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
ericsnowcurrently opened this issue Aug 1, 2019 · 5 comments
Closed

Discovered tests may have problematic filenames. #6820

ericsnowcurrently opened this issue Aug 1, 2019 · 5 comments
Labels
area-testing bug Issue identified by VS Code Team member as probable bug

Comments

@ericsnowcurrently
Copy link

The testing adapter returns the following data for discovery (one item per root):

[
    {
        "rootid": "<string>",
        "root": "<dirname>",
        "parents": [
            {
                "id": "<string>",
                "kind": "<enum-ish>",
                "name": "<string>",
                "parentid": "<string>"
            }
        ],
        "tests": [
            {
                "id": "<string>",
                "name": "<string>",
                "source": "<filename>:<lno>",
                "markers": ["<string>"],
                "parentid": "<string>"
            }
        ]
    }

For each root there is one property that holds a directory path ("root", AKA the test root). For each test there is one property that contains a filename ("source"). In both cases the extension cares about how the value maps (respectively) to the current workspace folder (or under it) and to a file known to the extension under that workspace root. (We could support test roots under other workspace folders, in the multi-root case, or even outside the workspace. However, that's outside the scope of this issue.)

The problem is that we have no guarantee that the test root will be under the current workspace root, nor that any test's source file will be a file known to the extension. Furthermore, there is not guarantee that either will have the same casing as the filename known to VS Code, on case-insensitive file systems.

Most of the time this isn't an issue. However, in the case of pytest, plugins can generate filenames that break our assumptions. On top of that, from what I can tell test roots outside the workspace are ignored and normcase issues only result in an extra editor window getting opened. So the consequences aren't severe.

The solution for the test root is along the lines of what was merged for #6755 (and later reverted in #6780). In src/client/testing/common/services/discoveredTestParser.ts we try to match data.root to the workspace root (or one of them for multi-root), either exactly or as a subdirectory, ignoring case.

The solution for each test's "source" is trickier since doing it in TestDiscoveredTestParser.parse() would require an MxN comparison between the tests and every file known to VS Code. And if VS Code doesn't have an API to quickly identify files in the workspace (either a list or some sort of workspace.hasFile()) then we would have to walk the filesystem. Either way it would probably be too expensive. So the check likely has to be done at each of the sites where the test source filename is actually used. This is how it is done for code lenses in #6782 (to solve #6303). Extending that to all other similar sites, possibly using a new helper, would be necessary. The solution isn't ideal since it any new consumers of the test data don't automatically get the check, but it's probably the best we can do.

@ericsnowcurrently ericsnowcurrently added bug Issue identified by VS Code Team member as probable bug area-testing needs decision labels Aug 1, 2019
@ericsnowcurrently
Copy link
Author

FWIW, handling the test root part (and ignoring the harder per-test part) may be sufficient. A solution similar to #6755 would be enough.

@PhilMacKay
Copy link

PhilMacKay commented Aug 7, 2019

Hello @ericsnowcurrently, would a fix to this issue also fix issues like #6410 and #6695 ? I hoped the fix #6782 would resolve the whole problem and allow us to run tests individually, but it only adds code lenses back (they are plagued by the same problem as the buttons in the test explorer).
All of my team's tests are located in folders and/or files with upper case here and there, and I would prefer not to move our hundreds of tests to new directories and files with all lower cases!

The whole behavior seems related to the fact that the adapter normcase paths returned by pytest since version 2019.6.0 (25 June 2019). To solve the issue, I changed the "NORMCASE" method in ~.vscode\extensions\ms-python.python-2019.8.29288\pythonFiles\testing_tools\adapter\pytest_discovery.py in class TestCollector.
Changing NORMCASE = staticmethod(os.path.normcase) to NORMCASE = staticmethod(os.path.normpath) fixes my issues, but it probably has other effects elsewhere of which I am not aware.
Could "normcase" be used on non-Windows os, and "normpath" be used on Windows?

UpperCaseBugs

Otherwise, I love how improvements to the extensions have been rolling in constantly for the last years!

@ericsnowcurrently
Copy link
Author

@PhilMacKay, issues #6410 and #6695 should be fixed now in the insiders build of the extension. (Feel free to validate.) This issue is more about some corner cases that are unlikely to affect most users.

@PhilMacKay
Copy link

PhilMacKay commented Aug 13, 2019

@ericsnowcurrently Awesome, thanks for the update :) I'll try it out as soon as I have some spare time! [edit: I just tried it on our main project and it works!]

@luabud
Copy link
Member

luabud commented Oct 29, 2019

This seems not to be a problem for users, so closing for now.

@luabud luabud closed this as completed Oct 29, 2019
@ghost ghost removed the needs decision label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-testing bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

3 participants