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

Fix assertions on exception messages #82

Merged
merged 5 commits into from
Apr 21, 2017
Merged

Fix assertions on exception messages #82

merged 5 commits into from
Apr 21, 2017

Conversation

KnVerey
Copy link
Contributor

@KnVerey KnVerey commented Apr 21, 2017

I realized while working on a feature on this repo that we've been misusing assert_raises. We have a bunch of tests that assume the second arg will be asserted as the error message, whereas it is in fact just used as the assertion failure message. I wrote most of these mistakes myself, but I don't think I'm the only one with the misimpression (it was actually originally suggested to me by a reviewer). This PR fixes the misuse, introduces a helper with the desired behaviour, and tries to prevent future confusion.

@KnVerey KnVerey requested review from kirs and sirupsen April 21, 2017 06:55
Copy link
Contributor

@kirs kirs left a comment

Choose a reason for hiding this comment

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

Personally I always use

error = assert_raises(MyError) {}
assert_match(/message/, error.to_s

But I see the point of assert_raises_msg and how it could be useful.


alias_method :orig_assert_raises, :assert_raises
def assert_raises(*args)
case args.last
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the value of the override but we should keep it mind that we could no longer use the last argument feature (http://www.rubydoc.info/gems/minitest/5.10.1/Minitest/Assertions#assert_raises-instance_method):

assert_raises(WhateverError, "does not raise when logging is disabled") { ... }

@KnVerey
Copy link
Contributor Author

KnVerey commented Apr 21, 2017

I see the value of the override but we should keep it mind that we could no longer use the last argument feature

True, and it would be easy enough to make that possible. Pushed another commit that lets you use a kwarg if you're using that feature on purpose. LMK what you think. Here are some examples:

  def test_assert_raises
    assert_raises(StandardError, "foobar") do
    end
  end

  1) Failure:
RunnerTest#test_assert_raises [test/unit/kubernetes-deploy/runner_test.rb:10]:
Please use assert_raises_msg to check the exception message,
or use the msg keyword argument to specify a message to use when the assertion fails.
  def test_assert_raises
    assert_raises(StandardError, msg: "This is an error message.") do
    end
  end

  1) Failure:
RunnerTest#test_assert_raises [test/unit/kubernetes-deploy/runner_test.rb:10]:
This is an error message.
StandardError expected but nothing was raised.
  def test_assert_raises
    assert_raises(StandardError) do
    end
  end

  1) Failure:
RunnerTest#test_assert_raises [test/unit/kubernetes-deploy/runner_test.rb:10]:
StandardError expected but nothing was raised.

@kirs
Copy link
Contributor

kirs commented Apr 21, 2017

Great idea!
What about using message instead of msg? That would keep parity with other test helpers at Shopify that use message as a keyword argument.

@KnVerey
Copy link
Contributor Author

KnVerey commented Apr 21, 2017

Pushed another commit that makes this a duplicate of what I just proposed in core.

Copy link
Contributor

@kirs kirs left a comment

Choose a reason for hiding this comment

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

🚢

@KnVerey KnVerey merged commit 6f41fb6 into master Apr 21, 2017
@KnVerey KnVerey deleted the raises_msg branch April 21, 2017 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants