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

Size matchers to provide insightful debugging information #294

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

Resolving my own #174.

I'm leaving the commits separate for easier reviewing, I'll squash them afterwards.

Briefly, the solution introduces a base class (I called it SizeMatcher) for the matchers which match on the size of a collection-like object.
Although, I have to say, I'm not a huge fan of complex hierarchies (low coupling, you know), but it made perfect sense to align with the current design. Maybe that could be a separate topic to discuss, if you think it's something which could be discussed.

One thing to note is the naive algorithm I'm using to generate the correct indefinite article "a"/"an", in SizeMatcher#indefiniteArticle.
I guess you must have already bumped into this problem, so if you already have a method to solve it more elegantly, I'll be more than happy to use it.

Please check the commit messages for more details on the changes (I had to update a few tests).

Thanks

PS: here I didn't take into account the "problem" for huge collections, which I was mentioning at the end in #174. I think we should not worry about that. The output may get cluttered, but that's the way it is, if you have a huge collection. Not our problem.

@sf105
Copy link
Member

sf105 commented Apr 19, 2020

This is an interesting idea. The reason we didn't show the actual collection contents originally is that we had no idea how big it would be (could be very large in some cases), so we left it out. Also, where there are compound matches on a collection, each one might dump out the whole contents.

I can see your point, but I'm not convinced that the removed duplication is worth the extra code. My experience is that the flakiness of the Java APIs sometimes justifies some clumsy code.

@alb-i986
Copy link
Contributor Author

This is an interesting idea. The reason we didn't show the actual collection contents originally is that we had no idea how big it would be (could be very large in some cases), so we left it out. Also, where there are compound matches on a collection, each one might dump out the whole contents.

Cool, so if you agree that it is a concern, shall we add a truncation logic as I was suggesting in #174 ?

I can see your point, but I'm not convinced that the removed duplication is worth the extra code. My experience is that the flakiness of the Java APIs sometimes justifies some clumsy code.

You mean the SizeMatcher? You prefer duplicating the code in every IsXXXWithSize class?
Between adding a layer to a complex hierarchy and duplicating, I prefer the former, in this case.
Because

  1. we're talking about duplicating code in 4 classes, not 1 or 2
  2. It just fits perfectly in the current design
  3. I said "complex hierarchy" but it was not that complex before my change, and it isn't after. It's still pretty much readable, understandable. Not the worst hierarchy I've seen.

My opinion is that if you want to re-think the design, I'm up for it. But until then, let's stick with what is already in place.

@nhojpatrick
Copy link
Member

@alb-i986 looks good, i've created a custom matcher myself, so the output reports what is in the collection is it is the wrong size. 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.

Now, matchers like hasSize and similar, in case of mismatch,
print also the actual values contained in the collection being tested.

Closes hamcrest#174
@alb-i986
Copy link
Contributor Author

Rebased and squashed. Ready to be merged.
Thanks.

@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