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

Use NameError#name to assert raised error. #11993

Merged
merged 1 commit into from Jul 11, 2014
Merged

Use NameError#name to assert raised error. #11993

merged 1 commit into from Jul 11, 2014

Conversation

razielgn
Copy link
Contributor

This makes the test compatible with other Ruby implementations, which may implement error messages differently.

@@ -319,7 +319,7 @@ def test_render_partial_without_object_or_collection_does_not_generate_partial_n
exception = assert_raises ActionView::Template::Error do
@controller_view.render("partial_name_local_variable")
end
assert_match "undefined local variable or method `partial_name_local_variable'", exception.message
assert_equal :partial_name_local_variable, exception.original_exception.name

Choose a reason for hiding this comment

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

I'm happy to merge it, but first let me ask: can this guarantee what the test is supposed to check? It checks that a local variable is not generated, which was clear in the message before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks that a local variable is not generated, which was clear in the message before.

I'm not sure about that, but all NameError subclasses (including NoMethodError) respond to #name� (since 1.9), so this should be keeping the original intention.
Could you give me more background about this test?

@razielgn
Copy link
Contributor Author

razielgn commented Oct 5, 2013

Any news on this? I'd love to see this merged, if it's not 100% OK I can improve it. :)

This makes the test compatible with other Ruby implementations, which
may implement error messages differently.
@kdaigle
Copy link

kdaigle commented May 18, 2014

This seems a little less clear than the change that was similarly merged in 0bc95ed. Happy to help get this finished up or closed out though. ✨

@razielgn
Copy link
Contributor Author

Hi Kyle, thanks for looking at this.
Given exception messages thrown "under the cover" by Ruby are always meant for humans and NameError exposes the missing variable/method which originated the exception via the #name method, I think it'd be totally reasonable to make assertion on what #name returns, especially since other Ruby implementations might decide to use different wording for the exception message.
I hope I was clear enough. :)

@kdaigle
Copy link

kdaigle commented May 18, 2014

@razielgn seems reasonable. Just adding my two cents to hopefully help @carlosantoniodasilva get this merged or closed. 😀

@rafaelfranca
Copy link
Member

cc @robin850

@robin850
Copy link
Member

robin850 commented Jun 4, 2014

@razielgn : Thanks a lot for your patch! Could you please add an assertion to make sure that the exception is an instance of NoMethodError ?

@robin850
Copy link
Member

robin850 commented Jul 6, 2014

@razielgn : Any chance to update your patch please ? :-)

guilleiguaran added a commit that referenced this pull request Jul 11, 2014
…t-error

Use NameError#name to assert raised error.
@guilleiguaran guilleiguaran merged commit e454a3d into rails:master Jul 11, 2014
@guilleiguaran
Copy link
Member

Merging per @robin850 comment ("Asking to assert that this was a NoMethodError was useless as it's an ActionView::Template::Error, sorry for the noise here")

matthewd added a commit that referenced this pull request Jul 12, 2014
Just so it's clearer what's going on in the following assertion.

/cc #11993 @robin850
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

6 participants