Skip to content
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

[DomCrawler] Check all matching elements in test assertions instead of only the first #36158

Closed

Conversation

@wouterj
Copy link
Member

wouterj commented Mar 21, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix symfony/symfony-docs#13385
License MIT
Doc PR -

@arneee reported in the docs repository that the assertSelectorTextContains() and assertSelectorTextSame() are only checking the first matching element. To me, this seems to be a bug (it's not what I would expect).

Meanwhile, I couldn't resist updating the tests a bit to use a data provider, instead of testing 3 things within one single test method.

@wouterj wouterj changed the title Doc 13385/assert selector contains [DomCrawler] Check all matching elements in test assertions instead of only the first Mar 21, 2020
@wouterj wouterj force-pushed the wouterj:doc-13385/assert-selector-contains branch from cdc4ab2 to 5317904 Mar 21, 2020
@wouterj wouterj force-pushed the wouterj:doc-13385/assert-selector-contains branch from 5317904 to 1cd9b90 Mar 21, 2020
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Mar 23, 2020
@wouterj wouterj force-pushed the wouterj:doc-13385/assert-selector-contains branch 2 times, most recently from 1facca8 to ea74fd2 Mar 23, 2020
return $this->expectedText === trim($crawler->text(null, false));
foreach ($crawler as $node) {
if ($this->expectedText === trim($node->text(null, false))) {
return true;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 23, 2020

Member

for "Contains" I get it's fine to have an "OR" match, but for "Same", shouldn't we expect one single node?

This comment has been minimized.

Copy link
@arneee

arneee Mar 23, 2020

I would not expect that it matches the first node only. If I would need that as an requirement, I would add this as a separate assert (with a count probably). I also would expect that assertSelectorTextSame and assertSelectorTextContains behave the same, beside of same/contains logic.

I had this problem when I wanted to check if the page contained a given headline, so I was looking for h2 tags with the text "Headline 3". For that check I don't care if there are more h2 tags with "Headline 2" or "Headline 1".

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 23, 2020

Member

I meant "the only one", not "the first".
i.e. when there are two matching nodes, it would fail.
Otherwise, what does "Same" mean?

This comment has been minimized.

Copy link
@arneee

arneee Mar 23, 2020

I always understood that Same checks for the whole node value, Contains for a part of it.
Example <p>Hello world</p>
Contains with selector "p", text "world" would succeed, Same would fail.

This comment has been minimized.

Copy link
@wouterj

wouterj Mar 23, 2020

Author Member

I agree with @arneee .

Same/contains are imho just selectors, they don't indicate something about the number of times it should match. So in that sense, they would act the same as Xpath selectors:

This comment has been minimized.

Copy link
@arneee

arneee Mar 23, 2020

When writing tests what would really confuse me... contains / same should imho behave the same regarding number of matches, the only difference should be how the actual node value is checked. Otherwise it is definitely not self-explanatory.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 23, 2020

Member

Here, we're not designing a new API. We're trying to fix an existing one.
Changing a behavior should not be taken lightly.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 23, 2020

Member

Fixing the doc might be the better option. Please consider twice.

This comment has been minimized.

Copy link
@wouterj

wouterj Mar 23, 2020

Author Member

What about adding an $exactTimes parameter that defaults to 1?

The issue now is that if we do assertSelectorTextContains('h3', 'Hello World!') and we have e.g. a <h3>Homepage</h3> in the header (before the <h3>Hello World!</h3> in the body), we can't use any of these nice selectors. This is both not nice DX and unexpected behavior imho.

However, I see your point that any change would deviate from the previous behavior and tests that would fail before, can pass now.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 23, 2020

Member

Adding an argument is a new feature :)

@wouterj wouterj force-pushed the wouterj:doc-13385/assert-selector-contains branch from ea74fd2 to 3ba6e17 Mar 23, 2020
@m-unkel

This comment has been minimized.

Copy link

m-unkel commented Mar 23, 2020

duplicate pull request #36062

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 31, 2020

I think we should reject this PR. As explained, this is not a bugfix anymore but a BC break to me. If we want to change the behavior here, we'd better have a proper deprecation before.

Meanwhile, let's update the doc.

@wouterj

This comment has been minimized.

Copy link
Member Author

wouterj commented Mar 31, 2020

Yes, agreed. It can't be done without breaking.

I would love to have better assertions for contains and same as a feature though, I don't think their current behavior makes sense. However, I'm not going to work on it. So anyone should feel free to start a PR :)

@wouterj wouterj closed this Mar 31, 2020
@wouterj wouterj deleted the wouterj:doc-13385/assert-selector-contains branch Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.