nathanboktae/chai-dom

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

alencar13 opened this issue · 10 comments

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?

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?

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.

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.

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')

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).

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....

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')

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

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.

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