Skip to content
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

Initial attempt at #7012; strip falsey values from strargs #7081

Closed

Conversation

symonk
Copy link
Member

@symonk symonk commented Apr 12, 2020

An attempt at fixing #7012, honestly I am not sure if this is the correct place to be doing this, the code base is completely new to me and pretty vast, feedback here is greatly appreciated by someone who knows the code base nicely.

I was able to recreate the issue of duplicate collection and my newly added tests confirms the fixed behaviour, regression suite remains passing.

Thanks for your time, have a great day and I look forward to your feedback.

  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

@symonk
Copy link
Member Author

symonk commented Apr 12, 2020

"args" seems to have some immutability (provided by @attr (new to me)), should we do this check somewhere much sooner?

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @symonk!

Checking only the empty string however is not sufficient -- for example,

$ pytest --collect-only '' '.' './.' 'tests.py' './tests.py' '././tests.py'
============================================================================ test session starts =============================================================================
platform linux -- Python 3.8.2, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /home/ran/tmp/test
collected 4 items                                                                                                                                                            
<Module tests.py>
  <Function test_01>
<Module tests.py>
  <Function test_01>
  <Function test_01>
  <Function test_01>

This makes me think that the level of strargs is probably not the right place to handle this.

@symonk
Copy link
Member Author

symonk commented Apr 13, 2020

@bluetech excellent thanks for the feedback, I will go back and investigate :) any recommendations on the best place for handling it?

I am not so sure its truly a bug, but rather expected behaviour, for example:

pytest foo.py foo.py would collect all tests in foo.py twice, which i think is intentional?

plugin here: https://github.com/nicoddemus/pytest-drop-dup-tests.
#1187
nicoddemus/pytest-drop-dup-tests#2

@bluetech
Copy link
Member

any recommendations on the best place for handling it?

Not out of hand, though I can look if needed.

I am not so sure its truly a bug, but rather expected behaviour, for example:

Right, good find! According to #1187, this is expected behavior, though it does seem a little odd to me. On the other hand, if you don't want to run a test more than once, don't specify it more than once 😃

Perhaps the right move for now is for the reporter to move to reopen the issue.

@bluetech
Copy link
Member

Actually, it seems like that decision was overturned in #1609 and the behavior was changed to remove duplicate tests by default, with a --keep-duplicates flag to disable it. So it does seem like a bug.

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

@symonk
Copy link
Member Author

symonk commented Apr 13, 2020

Free up again, will investigate a better solution. did some reading on the links provided - thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants