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

Pass assert_dom_equal message arg to underlying assertion #11751

Merged
merged 1 commit into from Aug 4, 2013

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Aug 4, 2013

#assert_dom_equal and #assert_dom_not_equal both take a "failure" message argument, but this argument was not utilized.

@@ -7,20 +7,20 @@ module DomAssertions
#
# # assert that the referenced method generates the appropriate HTML string
# assert_dom_equal '<a href="http://www.example.com">Apples</a>', link_to("Apples", "http://www.example.com")
def assert_dom_equal(expected, actual, message = "")
def assert_dom_equal(expected, actual, message = nil)
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference of changing this to nil?

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert_equal defaults the failure message argument to nil, and if we're going to pass it along, I thought it would be better to keep the same default so we're never affected by implementation changes in assert_equal.

@rafaelfranca
Copy link
Member

A test case would be great

@rafaelfranca
Copy link
Member

Could you add a CHANGELOG entry?

@rmm5t
Copy link
Contributor Author

rmm5t commented Aug 4, 2013

A test case would be great

@rafaelfranca Agreed, but I didn't see any tests for assert_dom_equal in ActionPack as it stands now. Should I just modify an existing test case in ActionView to utilize this argument or would you like to see real unit tests for assert_dom_equal created in ActionPack?

#assert_dom_equal and #assert_dom_not_equal both take a "failure"
message argument, but this argument was not utilized.
@rmm5t
Copy link
Contributor Author

rmm5t commented Aug 4, 2013

Could you add a CHANGELOG entry?

@rafaelfranca No problem. Changelog entry added and squashed.

@rafaelfranca
Copy link
Member

I just saw @kaspth already did the same changes on #11218.

I'll merge and work to add these tests.

rafaelfranca added a commit that referenced this pull request Aug 4, 2013
Pass assert_dom_equal message arg to underlying assertion
@rafaelfranca rafaelfranca merged commit 45357c5 into rails:master Aug 4, 2013
rafaelfranca added a commit that referenced this pull request Aug 4, 2013
Pass assert_dom_equal message arg to underlying assertion
Conflicts:
	actionpack/CHANGELOG.md
@rmm5t rmm5t deleted the assert_dom_equal-message branch August 4, 2013 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants