<DOMDocument|DOMElement|DOMDocumentFragment> [not] to contain <DOMElement|object>

[sunesimonsen]

Fixes: #254

[coveralls]

Pull Request Test Coverage Report for Build 1063

• 130 of 130 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+2.2%) to 93.75%

---

Totals
Change from base Build 1014:	2.2%
Covered Lines:	649
Relevant Lines:	677

---

💛 - Coveralls

[papandreou]

Looks promising!

Would be nice with a not to contain variant, but I guess that would require a different kind of diff to be implemented?

Is it intentional that the assertion passes when the subject itself matches the spec? There's no test that covers it, so I can't really tell. This passes:

expect(
  parseHtmlNode('<span id="foo"></span>'),
  'to contain',
  '<span id="foo"></span>'
);

I'd argue that it doesn't really contain a match in that case. On the other hand, I don't really know what would be more useful.

The support for a RHS string seems broken:

expect(parseHtmlNode('<span>hello</span>'), 'to contain', 'hello');

Error: Unsupported options: 0, 1, 2, 3, 4

[sunesimonsen]

@papandreou thanks for the feedback.

Is it intentional that the assertion passes when the subject itself matches the spec? There's no test that covers it, so I can't really tell. This passes:

It was, but looking at to contain elements matching I can see that it only looks at the descendant elements, so this assertion should behave the same. I'll fix that.

The support for a RHS string seems broken

This is a bit more tricky, as you would normally do something like this:

expect(
  parseHtmlNode('<div><span class="greeting">Hello</span><span class="name">Jane Doe</span></div>'), 
  'to contain', 
  '<span class="name">Jane Doe</span>'
);

But I think it would be nice if this also worked:

expect(
  parseHtmlNode('<div><span class="greeting">Hello</span><span class="name">Jane Doe</span></div>'), 
  'to contain', 
  'Jane Doe'
);

I'll see what I can do.

[alexjeffburke]

I've done some thinking about the semantics and I tend to agree with Andreas - one of the things we know from the past is overly intelligent matching has almost always come to bite us. In this case, it feels right that "to contain" matching should perhaps require children or an array before it will go searching for a match - and it makes it much easier to explain when to use this versus using "to satisfy" on an element.

About the string matching, hmm - I'm not sure. It would be nice it works but again I'm a bit worried that it becomes a bit magic - the behaviouraround what will be found needs to be super clear. By that reasoning the semantics of "... to contain <string>" would be find a text node where that entire string was its content and nothing else. Does that make sense?

[sunesimonsen]

@alexjeffburke and @papandreou I fixed both the cases you pointed out.

[sunesimonsen]

@papandreou about not to contain, we can just use the current code to see if we can find a good match, if that matches the assertion should fail. For the diff we could try to clone the subject and remove all matching elements (or maybe only the best one) and use to satisfy against the modified clone. I think that will give you, should be removed.

[alexjeffburke]

@sunesimonsen I think the removal of the text support is a very good thing - it'll keep the semantics clean and just sidestep any pontetial surprise factor.

[papandreou]

Looks great, just some small nitpicks.

[papandreou]

Just for full disclosure -- IMO it'd be fine to support a RHS of a string without HTML tags wrapped around it -- as long as < etc. are interpreted as HTML rather than text.

We can also add that later, so nbd.

[sunesimonsen]

@papandreou I'm leaning towards having an explicit
to contain text assertion, that also mirror the current to have text and to satisfy assertions better. What du you think about that idea?

[papandreou]

That could be a fine addition of its own. I still think <DOMDocument|DOMElement|...> to contain <string> should behave as I suggest above.

[sunesimonsen]

But I think that is what we had, but I saw some weirdness around HTML entities so I just reverted it. Let's see if we can reintroduce it in a later PR.