nathanboktae / chai-dom

DOM assertions for the Chai assertion library using vanilla JavaScript
Other
76 stars 26 forks source link

I expected `contains` to change the assertion subject to the matched element #20

Closed alencar13 closed 7 years ago

alencar13 commented 7 years ago

I’m having some troubles with an implement of chai-dom when I assert child elements.

I have this code:

describe('attribute readonly', () => {
it('should add attribute to input child', () => {
   element.setAttribute('readonly','readonly')
   // expect(element).to.contain('input').to.have.attribute('readonly')

   // temporary assertion, because above not work, need to be refactor
   expect(element.querySelector('input')).to.have.attribute('readonly')
})

Is it possible write an assertion to child input? Or .contain don't be designed to that?

nathanboktae commented 7 years ago

The test doesn't make sense. You set readonly on element, then assert an input that is a child element has readonly ? how did it get that?

alencar13 commented 7 years ago

It's an assertion to a webcomponent, so, the element has some abstractions.

What I really want is assert if a input child of element has a specific attribute.

I want assert if a input inside another, has an attribute, writing something similar to:

expect(element).to.contain('input').with.attribute('readonly')

but broke, because .with.attribute assert element instead input child.

nathanboktae commented 7 years ago

You expected the contain to change the assertion subject to the element matched, correct? The question is does everyone else expect that and is that clear? For example:

element.should.contain('input').and.have.attribute('readonly') // who should have the attribute? the parent or child?
element.should.contain('input').that.has.attribute('readonly') // more clearly talking about the child
element.should.contain('input').and.contain('label') // breaks if we take your suggestion of changing assertion subjects. e.g. is it div>input+label or div>input>label when this passes.
nathanboktae commented 7 years ago

How about we add child that is the same as contains but changes the subject without braking existing usages? so you can do

expect(element).to.have.child('input').that.has.attribute('readonly')
alencar13 commented 7 years ago

Exactly! I agree with contain can change the assertion contexts and it is clear for everyone.

In respect to

expect(element).to.have.child('input').that.has.attribute('readonly')

I think no viable to use child because the principal idea is taking the decendants of the element, and in the case of child just is possible taking the first decendant (child).

nathanboktae commented 7 years ago

Exactly! I agree with contain can change the assertion contexts and it is clear for everyone.

No I don't agree and it's a breaking change. Everyone currently using this library and contains use it with keeping the same assertion subject.

Yeah good point, child usually is a direct descendant. hmm....

nathanboktae commented 7 years ago

I'll add a descedant assertion assertion that does what contain does but changes the subject, to support both cases of wanting to, or not to, change the assertion subject:

div.should.contain('input').and.contain('label')
div.should.have.descendant('input').that.has.attribute('readonly', 'readonly')
darlanmendonca commented 7 years ago

why dont use .contain instead create a new chainable?

really is better create a new chainable .descendant to that? (search and change the context), while .contain only search and search for descendant too? (but without change the context)

looks a little confuse to me

nathanboktae commented 7 years ago

why dont use .contain instead create a new chainable?

As stated before, it's a breaking change, and if you write an assertion like this:

// DOM: div.wrapper>label+input
document.querySelector('div.wrapper').should.contain('label').and.contain('input')

it clearly expects it to NOT change the assertion subject and chain multiple assertions off the original element. It's all a matter of style.

darlanmendonca commented 7 years ago

yeah you're right, with chainable and any change in context will be a breaking change