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

Fixed error where filename too long being not caught #10169 #10172

Closed
wants to merge 8 commits into from
Closed

Fixed error where filename too long being not caught #10169 #10172

wants to merge 8 commits into from

Conversation

PurityLake
Copy link
Contributor

@PurityLake PurityLake commented Jul 27, 2022

head-fork: PurityLake/pytest
compare: filename-length-fix

base-fork: pytest-dev/pytest
base: main

Refers to #10169

I simply added a except statement to catch the OSError

Also just noticed a fairly large typo in the commit message.

@PurityLake PurityLake changed the title Fixed error where filename being error not caught #10169 Fixed error where filename too long being not caught #10169 Jul 27, 2022
@jdckmz
Copy link

jdckmz commented Jul 27, 2022

This doesn't fully resolve the problem. It works if the user was trying to specify a file, but If you define a custom flag and that flag is too long pytest fails with your error instead of working. If you change to

            except OSError:
                pass # Assuming this isn't a file

that fixes my use case, but I don't know if it will cause breakage in other cases

@PurityLake
Copy link
Contributor Author

So what should be done if the arg is not a path or file as in your case?

It seems in this case it is assumed that at this point a path is assumed.

@jdckmz
Copy link

jdckmz commented Jul 28, 2022

Yes. I don't know why arguments are being assumed to be paths when they're not. I put more details in my comment on the bug report #10169 (comment)

@PurityLake
Copy link
Contributor Author

PurityLake commented Jul 28, 2022

Well from what I can tell you can make a fixture in conftest.py to fix your problem like so:

@pytest.fixture
def xxxxx_flags(request):
    return request.config.getoption("--xxxxx_flags")

This seems to stop pytest from assuming it's a path but I believe you would then need to run your own command line parsing for something like that.

@PurityLake
Copy link
Contributor Author

Seems the tests are failing for Windows. The code coverage fails if I don't invoke the internal method but my test still covers the UsageError (without the newer part to the test). Is it ok to not invoke the internal method or should I work to fix this?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @PurityLake for tackling this.

Besides the comment about the approach and testing, this also needs a CHANGELOG entry. 👍

@RonnyPfannschmidt
Copy link
Member

im wondering, since the original issue is about passing long options that may be unknown,
should we perhaps ignore argument items that follow option syntax as separate detail

i do believe on actual too long filenames pytest should fail in any case as part of fail early

@PurityLake
Copy link
Contributor Author

@RonnyPfannschmidt I think the issue is, is that the argument needs to be declared prior to it getting to this point to not be considered a path. The part my tests cover is an actually filename but I believe I can add a quick check for a potential option but how should it be dealt with? Ignore it, or raise a UsageError?

@PurityLake PurityLake closed this Dec 25, 2022
@arichardson
Copy link

Well from what I can tell you can make a fixture in conftest.py to fix your problem like so:

@pytest.fixture
def xxxxx_flags(request):
    return request.config.getoption("--xxxxx_flags")

This seems to stop pytest from assuming it's a path but I believe you would then need to run your own command line parsing for something like that.

This does not seem to work for me since this Path.exists() is called before any of the conftest.py logic.

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.

None yet

5 participants