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

Fix feature matcher when feature throws #296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alb-i986
Copy link
Contributor

@alb-i986 alb-i986 commented Apr 19, 2020

While working on #294 I noticed that FeatureMatcher#featureValueOf may throw.
Right now, that may happen in:

  • HasToString, if the actual object has a custom toString method which throws
  • FileMatchers.aFileWithCanonicalPath and siblings

I'm gonna squash as soon as you review.

@sf105
Copy link
Member

sf105 commented Apr 19, 2020

Thanks for contributing. I disagree with this one because if there's an exception at this level, then it's not a mismatch, but something is deeply broken and we should stop immediately.

@alb-i986
Copy link
Contributor Author

Well, for the toString method, you have a point. But we can't say that in general.
Look at FileMatchers#aFileWithCanonicalPath: actualFile.getCanonicalPath() may throw IOException, which totally depends on the actual value. There's nothing deeply broken here.

@sf105
Copy link
Member

sf105 commented Apr 19, 2020

I would argue that that is different, because it reports a known possible failure that is in the domain of the matcher. For generic feature matchers, it's harder to make universal decisions. If you wanted to enforce consistency, I'd rather remove the exception handling from the file matcher.

What does the additional complexity handling of the exception do to improve the user's experience?

@alb-i986
Copy link
Contributor Author

I would argue that that is different, because it reports a known possible failure that is in the domain of the matcher.

This is really where we disagree. I believe it's a "user space" issue, instead. Not a Matcher problem.

It's the same concept as throwing in matchesSafely() if the method for matching throws.
We don't do that, of course. We try-catch and return false. See e.g. HasProperty

@nhojpatrick
Copy link
Member

@alb-i986 please can you rebase from master, as hamcrest-core and hamcrest-library have been refactored a lot and also deprecated, so that everything is just in hamcrest.

@alb-i986 alb-i986 force-pushed the fix-feature-matcher-when-feature-throws branch from f805546 to 67452d1 Compare August 23, 2020 15:36
Right now, that may happen in:
- HasToString, if the actual object has a custom toString method which may throw
- FileMatchers.aFileWithCanonicalPath and siblings
@alb-i986 alb-i986 force-pushed the fix-feature-matcher-when-feature-throws branch from 67452d1 to 97e8b93 Compare August 23, 2020 15:37
@alb-i986
Copy link
Contributor Author

Rebased and squashed. Ready to be merged.

@nhojpatrick
Copy link
Member

Going to try and kick start hamcrest, so if you want to get it merged, please rebase from the branch v2.3-candidates.
Still trying to understand how has permissions to perform a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: High priority
Development

Successfully merging this pull request may close these issues.

3 participants