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

Add DarcyWebMatchers with HasClass matcher #46

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add DarcyWebMatchers with HasClass matcher #46

wants to merge 3 commits into from

Conversation

dmcneil
Copy link
Member

@dmcneil dmcneil commented Feb 11, 2016

No description provided.

@dmcneil
Copy link
Member Author

dmcneil commented Feb 11, 2016

Thinking a bit more about this while at lunch, I wonder if it would be better to accept a Matcher<? super String> instead of just a String so you could perhaps do something like element, hasClass(containsString("cla")) on an element that had a class of clazz...

@dmcneil
Copy link
Member Author

dmcneil commented Feb 11, 2016

I decided that using the Matcher<? super String> allows for some more freedom/versatility.

@alechenninger alechenninger self-assigned this Feb 11, 2016

import org.hamcrest.Matcher;

public abstract class DarcyWebMatchers {
Copy link
Member

Choose a reason for hiding this comment

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

Probably worthwhile to extend darcy-ui's DarcyMatchers. This makes it easier to consume all matchers, and prevents name collisions (which is important because I imagine these will be ripe for static imports).

@dmcneil
Copy link
Member Author

dmcneil commented Feb 15, 2016

For clarification, are you saying either use TypeSafeDiagnosingMatcher on the matcher I have here as is or rewrite it to work off a Collection instead of an HtmlElement and then just pass in the List<String> you get from element.getClasses() in the assertion rather than using getClasses() in the matcher itself?

@alechenninger
Copy link
Member

The former: use the diagnosing matcher in the matcher you are writing.

The alternative is to not write another matcher at all, and just use the collections matchers already available against the Set from getClasses(). But, this has the downside of not being able to explain the context about the set: all it knows is that it was a set of strings. If you do this matcher and make it self-diagnosing, we can get more context about the failure, which is nice.

@dmcneil
Copy link
Member Author

dmcneil commented Feb 15, 2016

Oh yeah that makes complete sense, duh, the matchers already exist for collections. Uhh, yeah...brain still waking up for Monday. I'll get this fixed. Thanks for the quick response.

}

description.appendText("could not find a matching class in ")
.appendValue(t.getClasses());
Copy link
Member

Choose a reason for hiding this comment

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

Two improvements you can make with this and then we're good:

  1. Append the element to String and the getClasses -- again if we just use getClasses, then this is no better than using the already provided collection matchers
  2. Also use the matcher provided to describe what about a class you were looking for

@alechenninger
Copy link
Member

Hey @dmcneil :-). Any interest in working on those last two improvements?

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

Successfully merging this pull request may close these issues.

2 participants