-
Notifications
You must be signed in to change notification settings - Fork 6
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
Implement "not to have (class|classes)" assertion. #269
Conversation
Pull Request Test Coverage Report for Build 1134
💛 - Coveralls |
149ca61
to
6e1cff1
Compare
src/index.js
Outdated
removeValueFromArray(expectedClasses, namedValue) | ||
); | ||
|
||
return expect(subject, 'to only have classes', expectedClasses); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, couldn't you map it to:
const classAttributeValue = getAttributes(subject).class;
const actualClasses = typeof classAttributeValue === 'string' ? getClassNamesFromAttributeValue(classAttributeValue) : [];
const expectedClasses = Array.isArray(value) ? value : [value];
expect(actualClasses, 'not to contain', ...expectedClasses);
That would produce a nicer error message and could also cover the to have classes
case with a bit of flag forwarding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah could do - IIRC I did it this way for consistency something else in U-dom, I think it was https://github.com/unexpectedjs/unexpected-dom/blob/master/src/index.js#L1329. But happy to change it round if that works better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I looked at the test I thought it was confusing that it said:
class="bar quux" // expected [ 'bar', 'quux' ] to equal [ 'bar' ]
... when the user didn't actually mention bar
in the assertion.
If the other place is equivalent to that, I think we should change that as well 🤗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have pushed a commit to make the assertion run via "not to contain" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay, thanks! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
src/index.js
Outdated
@@ -99,6 +99,14 @@ function validateStyles(expect, str) { | |||
} | |||
} | |||
|
|||
function removeValueFromArray(array, value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh! Knew I left something - thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Don't we need docs? |
@sunesimonsen you were absolutely right and that should have been in there :) |
Closes #201 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 great thanks
No description provided.