Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Added fix and unit test for queryContentRegexAssertion() method #33

Closed
wants to merge 2 commits into from

Conversation

RalfEggert
Copy link
Contributor

Solves issue for #32.

$nodeValues[] = $node->nodeValue;
}

if (!preg_match($pattern, implode('', $nodeValues))) {
Copy link
Member

Choose a reason for hiding this comment

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

The concern I have with this approach is that the regex might match across content boundaries.

As an example, consider this markup:

<div>foo</div>
<div>bar</div>

and then the following assertion:

$this->assertQueryContentRegex('div', '/foobar/');

The above, with the change you suggest here, would be a false positive.

I'd suggest doing the regex in the loop itself:

$found = false;

foreach ($result as $node) {
    if (preg_match($pattern, $node->nodeValue)) {
        $found = true;
        break;
    }
}

if (! $found) {
    throw new PHPUnit_Framework_ExpectationFailedException(/* ... */);
}

$this->assertTrue($found);

This approach will resolve the issue you reported in #32, while ensuring we
don't get false positives due to boundary issues.

@RalfEggert
Copy link
Contributor Author

@weierophinney

I added another test case to handle unexpected false positives and added for solution. I had to tweak it a little otherwise the exception message had to actual content to output. I hope that tweak is ok.

weierophinney added a commit that referenced this pull request Sep 6, 2016
weierophinney added a commit that referenced this pull request Sep 6, 2016
@weierophinney
Copy link
Member

Thanks, @RalfEggert! Cherry-picked to master, for release with 3.0.2.

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

Successfully merging this pull request may close these issues.

2 participants