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

Fixed #24112 -- Fixed counting of elements if needle has no root element #7079

Merged
merged 1 commit into from Sep 1, 2016

Conversation

Projects
None yet
2 participants
@adamzap
Contributor

adamzap commented Aug 13, 2016

This PR has the fixes suggested by @timgraham in #4041:

  • Use assertRaisesMessage instead of assertRaises
  • Print the element with the exception message to give context for debugging
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 15, 2016

Member

One of the comments on the previous PR was "Should we add some context to the message (like part of the HTML snippet) to help identify the needle? I'm a bit worried developers will have a tough time figuring out what the message means." Any thoughts on that?

Member

timgraham commented Aug 15, 2016

One of the comments on the previous PR was "Should we add some context to the message (like part of the HTML snippet) to help identify the needle? I'm a bit worried developers will have a tough time figuring out what the message means." Any thoughts on that?

@adamzap

This comment has been minimized.

Show comment
Hide comment
@adamzap

adamzap Aug 15, 2016

Contributor

Yeah, I decided to print element in the message with the ValueError that gets raised. Is that sufficient?

https://github.com/django/django/pull/7079/files#diff-227f11729ee2a09bf3a4f7ca62f54ffbR99

Contributor

adamzap commented Aug 15, 2016

Yeah, I decided to print element in the message with the ValueError that gets raised. Is that sufficient?

https://github.com/django/django/pull/7079/files#diff-227f11729ee2a09bf3a4f7ca62f54ffbR99

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 15, 2016

Member

Oops, I didn't notice that because it's not part of the message checking in the test. I would have phrased it like "Needle HTML <HTML snippet> doesn't have a root element." but I think the message should also answer the questions, "why is this a problem?" and "what can you do to fix it?"

Member

timgraham commented Aug 15, 2016

Oops, I didn't notice that because it's not part of the message checking in the test. I would have phrased it like "Needle HTML <HTML snippet> doesn't have a root element." but I think the message should also answer the questions, "why is this a problem?" and "what can you do to fix it?"

@adamzap

This comment has been minimized.

Show comment
Hide comment
@adamzap

adamzap Aug 15, 2016

Contributor

Good point, is that better?

Contributor

adamzap commented Aug 15, 2016

Good point, is that better?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 15, 2016

Member

It still doesn't really explain why that's an error and what should be done to solve it. I'm particularly thinking about an inexperienced who sees that message.

Member

timgraham commented Aug 15, 2016

It still doesn't really explain why that's an error and what should be done to solve it. I'm particularly thinking about an inexperienced who sees that message.

@adamzap

This comment has been minimized.

Show comment
Hide comment
@adamzap

adamzap Aug 15, 2016

Contributor

Maybe now? 😉

If the line length is a problem, let me know how I should fix it.

Contributor

adamzap commented Aug 15, 2016

Maybe now? 😉

If the line length is a problem, let me know how I should fix it.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 15, 2016

Member

Sorry for the confusion. The question I have as a user would be "Why is a missing a root element problematic here?"

Member

timgraham commented Aug 15, 2016

Sorry for the confusion. The question I have as a user would be "Why is a missing a root element problematic here?"

@adamzap

This comment has been minimized.

Show comment
Hide comment
@adamzap

adamzap Aug 15, 2016

Contributor

No worries! I was thinking that the user calling assertInHTML and getting Needle HTML <HTML> is missing a root element from haystack HTML <HTML> is clear enough.

I'm happy to adjust it if I'm still misunderstanding you.

Contributor

adamzap commented Aug 15, 2016

No worries! I was thinking that the user calling assertInHTML and getting Needle HTML <HTML> is missing a root element from haystack HTML <HTML> is clear enough.

I'm happy to adjust it if I'm still misunderstanding you.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 26, 2016

Member

As we discussed on IRC, I'd like to know more about why the code has this requirement in the first place so the error message could explain that (assuming it's a reasonable requirement that doesn't make sense to change).

Member

timgraham commented Aug 26, 2016

As we discussed on IRC, I'd like to know more about why the code has this requirement in the first place so the error message could explain that (assuming it's a reasonable requirement that doesn't make sense to change).

@adamzap

This comment has been minimized.

Show comment
Hide comment
@adamzap

adamzap Aug 26, 2016

Contributor

Makes sense. I should have time to look at it this weekend.

Contributor

adamzap commented Aug 26, 2016

Makes sense. I should have time to look at it this weekend.

@adamzap

This comment has been minimized.

Show comment
Hide comment
@adamzap

adamzap Aug 29, 2016

Contributor

@timgraham Rather than try to merely clean up the original PR for this ticket, I've tried to fix the underlying problem 😄

If I'm understanding correctly, the problem is with this decision in _count.

Let's say we're checking if <p>foo</p><p>bar</p> is in <div><p>foo</p><p>bar</p></div>. The children of the haystack are <p>foo</p> and <p>bar</p>. So when _count is called on each of the children with the original needle, a match is not found. Comparing .children directly bypasses the separation that the loop causes. This is the exact test that this patch provides.

Contributor

adamzap commented Aug 29, 2016

@timgraham Rather than try to merely clean up the original PR for this ticket, I've tried to fix the underlying problem 😄

If I'm understanding correctly, the problem is with this decision in _count.

Let's say we're checking if <p>foo</p><p>bar</p> is in <div><p>foo</p><p>bar</p></div>. The children of the haystack are <p>foo</p> and <p>bar</p>. So when _count is called on each of the children with the original needle, a match is not found. Comparing .children directly bypasses the separation that the loop causes. This is the exact test that this patch provides.

@timgraham timgraham merged commit ca2ccf5 into django:master Sep 1, 2016

6 checks passed

default Build finished.
Details
docs Build finished.
Details
flake8 Build finished.
Details
isort Build finished.
Details
javascript Build finished.
Details
windows Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment