Skip to content

Check invalid operations for -k #6854

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

Merged
merged 1 commit into from
Mar 15, 2020
Merged

Conversation

gdhameeja
Copy link
Contributor

@gdhameeja gdhameeja commented Mar 3, 2020

#6822 KeywordMapping returns a bool on lookup which when passed to eval
fail on certain operations such as index access and attribute access.
Check has been added to raise a UsageError if such operations are passed
to -k

@gdhameeja
Copy link
Contributor Author

gdhameeja commented Mar 3, 2020

The change is very simple check to see if '.' or '[' exists in the keywordexpr, a UsageError is raised.
Please let me know if there is a better way to do this. Also kindly guide me on how I can write a test for this and if I need to add this information to the documentation that attribute access and indexing is not allowed in -k inputs.

@nicoddemus
Copy link
Member

Thanks a lot @gdhameeja, we appreciate the PR!

I replied here #6822 (comment) because I think we should discuss the approach further. 👍

@gdhameeja
Copy link
Contributor Author

@nicoddemus I checked out the comment #6822 (comment) and I understand why checking for '.' isn't such a great idea. I can reference a new PR for changing the expression so it passes eval.
As far as reporting is concerned, I did check for TypeError and raise the appropriate message. Can I just remove the checking for this PR and let the reporting be?

@bluetech
Copy link
Member

bluetech commented Mar 6, 2020

I think this is a good measure to fix the problem at least while we're still using eval.

I agree with @nicoddemus in the issue that we might as well do a full except Exception to catch all cases.

`KeywordMapping` returns a bool on lookup which when passed to eval
fail on certain operations such as index access and attribute access.
We catch all exceptions and raise a `UsageError`.
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.

So this seems like a good thing to have for now, until/if the eval goes away.

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.

Anybody up for backporting this? 😁

@gdhameeja
Copy link
Contributor Author

Anybody up for backporting this?

I'm up. Although I don't understand what you mean by backporting it and would need a little guidance.

@bluetech bluetech merged commit c26bbdf into pytest-dev:master Mar 15, 2020
@bluetech
Copy link
Member

I'm up. Although I don't understand what you mean by backporting it and would need a little guidance.

Since this change just missed the 5.4.0 release, it is merged to master which means it will only be included in 5.5.0 which can be some time from now. "Backporting" means applying the same change to the 5.4.x release series, which means it will be included also in the next 5.4.x patch release (e.g. 5.4.2).

In practice, you want to:

  • git fetch --all
  • git checkout 5.4.x
  • git checkout -B Fix-6822-backport
  • git cherry-pick 599bf075dbf1c1afb1a59b521dca52155c157488
  • Submit this Fix-6822-backport branch as a PR against the 5.4.x branch

We intend to automate this, but haven't yet...

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.

4 participants