Quotation marks in pytest command collects duplicate tests

[mikraj01]

• a detailed description of the bug or suggestion
  Extra single or double quotation marks ('' or "") in pytest command results in duplicate tests in pytest run

• pip list

Package            Version
------------------ -------
attrs              19.3.0 
importlib-metadata 1.6.0  
more-itertools     8.2.0  
packaging          20.3   
pip                20.0.2 
pkg-resources      0.0.0  
pluggy             0.13.1 
py                 1.8.1  
pyparsing          2.4.6  
pytest             5.4.1  
setuptools         46.1.3 
six                1.14.0 
wcwidth            0.1.9  
wheel              0.34.2 
zipp               3.1.0  

• pytest and operating system versions

pytest             5.4.1  
Description:	Debian GNU/Linux 10 (buster)
Release:	10
Codename:	buster

• minimal example if possible

# tests.py
def test_01():
    pass

pytest --collect-only tests.py ""
============================================================================= test session starts =============================================================================
platform linux -- Python 3.7.3, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /home/mikko/sandbox/pytest
collected 2 items                                                                                                                                                             
<Module tests.py>
  <Function test_01>
<Module tests.py>
  <Function test_01>

============================================================================ no tests ran in 0.01s ============================================================================

[RonnyPfannschmidt]

At first glance i believe '' is considereas '.'

Unfortunately i don't know if i can verify this weekend

[Zac-HD]

Yeah, I'd bet that we're using if not foobar: somewhere which should be if foobar is not None:, and so the empty string is treated as the default value.

[symonk]

Will investigate this one if thats ok

[symonk]

from what I gather we care not about file paths explicitly, regardless of --keep-duplicates e.g if you pass two paths to explicit .py files (not dirs) then keep duplicates doesn't even play a part, I think it is only used when we are dealing with directories. @RonnyPfannschmidt any idea where the best place at fixing this would be? I was looking in and around strargs, but I think its already too late by that point

[RonnyPfannschmidt]

Right now I have no idea, currently I'm also stepped away from the Project

[DahlitzFlorian]

I reproduced the behaviour and applied the changes @symonk suggested. I went one step further and added another test to show that duplicates are a desired effect:

def test_collection_duplicates_if_specified_twice(testdir):
    foo = testdir.makepyfile(foo="""
        def test_duplicate_if_specified():
            pass
    """
    )

    result = testdir.runpytest("./foo.py", ".", "--collect-only")
    result.stdout.fnmatch_lines(
        [
            "collected 2 items",
            "<Module foo.py>",
            "  <Function test_duplicate_if_specified>",
            "<Module foo.py>",
            "  <Function test_duplicate_if_specified>",
        ]
    )

I had a look at the _collectfile function in src/_pytest/nodes.py and played a little bit with it. I was not able to change it in a way that won't make other tests fail and the two tests related to this issue pass.

@bluetech I think strargs is the right place to handle it as nobody expects that adding empty quotes produces duplicated tests, whereas adding files or directories multiple times is intended to result in duplicated tests.

I would suggest to reopen the PR from @symonk (perhaps adding the test of this comment) and accept it. As an alternative I can open a PR if reopening is no option.

What are your thoughts on this?

[bluetech]

@DahlitzFlorian, in #7081 (comment) I wrote

My suggestion would be to look at --keep-duplicates (or rather what it disables) and find the bug there.

I don't remember all of the details, but have you looked at this?

[DahlitzFlorian]

I read your comment and had a look at it. I identified the _collectfile function to be the part handling that.

https://github.com/pytest-dev/pytest/blob/master/src/_pytest/nodes.py#L540

However, I was not able to modify it in a way that let the tests cases pass and won't break other test cases.

[bluetech]

However, I was not able to modify it in a way that let the tests cases pass and won't break other test cases.

From the link, it seems to currently rely on __hash__/__eq__ to detect duplicates (since it's using a set). I assume this is not sufficient? If something is changed, it should be changed here, I think.

[DahlitzFlorian]

The issue that I see is that the empty string ('' or "") becomes a LocalPath. As pytest does not handle duplicates by default, their is no way to see whether a LocalPath is the result of passing an empty string to pytest as command-line argument. With that said, the only way to remove duplicates occurring because of passing an empty string to pytest is to handle them at strargs. Other duplicates (occurring because of passing paths to it multiple times) won't be affected this way (as intended).

[bluetech]

@DahlitzFlorian So I think there two issues here, let me try to untangle them for myself at least:

---

Problem 1: pytest '' runs tests for the current directory, i.e. '' (the empty string) is treated as '.' (current directory). This is not generally expected, e.g.

$ ls ''
ls: cannot access '': No such file or directory

I think pytest should follow the common behavior and not treat the empty string as the current directory.

---

Problem 2: The duplicate detection (--keepduplicates not set, the default) only works for directories, not files. So for example

pytest tests tests/test.py
pytest tests/test.py tests/test.py

both run the tests in test.py twice.

This may be intentional, I'm not sure.

[DahlitzFlorian]

@bluetech Thanks for these insights. Indeed, it seems like '' is treated as '.'. But this is again a sign to filter it out using strargs (at least in my opinion). The fact that --keepduplicates only affects directories is another topic, which needs to be discussed and (maybe) fixed.
However, I'm eager to hear other peoples opinions on that, too.
@asottile @nicoddemus What do you think about it?

[asottile]

my 2c is that pytest '' should be an error and the duplicates based on test name is a separate issue entirely

[bluetech]

@DahlitzFlorian

Indeed, it seems like '' is treated as '.'. But this is again a sign to filter it out using strargs (at least in my opinion).

Yeah, for the first problem I agree. Looking at this, the conversion happens here:

pytest/src/_pytest/main.py

Line 616 in 70b5bdf

fspath = self.config.invocation_dir.join(relpath, abs=True)

So I think this function (Session._parsearg) is the right place to check for an empty arg.

[bluetech]

Forgot to mention that I agree with @asottile that it should be an error, and not silently skipped.

[DahlitzFlorian]

Sounds good. Today or tomorrow I will submit a PR addressing it.

[DahlitzFlorian]

My first attempt was to check in _parsearg whether or not arg == ''. If it's the case, pytest raises a UsageError otherwise proceed.

However, it turned out that this would make five of our tests fail as they use parametrize with the empty string to check the case where a certain option was not passed to pytest. A link to one of these tests is given below.

pytest/src/_pytest/main.py

Line 610 in 70b5bdf

def _parsearg(self, arg):

https://github.com/pytest-dev/pytest/blob/master/testing/logging/test_reporting.py#L870

[bluetech]

In the case of this particular test, I'd change the '' param to None, and skip passing it if None.

[DahlitzFlorian]

I guess, we need to check whether it was '' at the beginning and a path afterwards. This way, we can eliminate the test failures. I won't have the time to look at it until the end of the week.