Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Fix a nil error in failure_message of content type validation matcher #1910

Merged
merged 3 commits into from
May 24, 2016

Conversation

lime
Copy link
Contributor

@lime lime commented Jun 23, 2015

When a matcher uses both allowing and rejecting, and validation fails, @missing_rejected_types might still be nil. The validation itself works as intended, but calling failure_message raises due to @missing_rejected_types.any?.

Presumably, this fixes #1334. Possibly also #1476.


I haven't added any tests for this fix yet. Any suggestions on what tests we would want? The minimal case would be:

expect{ matcher.failure_message }.not_to raise_error

But that feels dumb. On the other hand, testing the failure message string for some specific pattern feels like overkill. Ideas?

@jyurek
Copy link

jyurek commented Jun 26, 2015

Can you add a test to demonstrate the error this may fix?

@lime
Copy link
Contributor Author

lime commented Jun 29, 2015

Can you add a test to demonstrate the error this may fix?

Sure, sure. As I wrote above, I'm requesting advice on how extensively we want to test the output of failure_message. However, I guess it's easier to discuss that if we have some starting point.

@lime lime force-pushed the content-type-matcher-nil branch from 97967f2 to dc6968f Compare June 29, 2015 07:11
end

it "does not run the validation if the control is false" do
dummy = Dummy.new
dummy.go = false
expect(matcher).to_not accept(dummy)
expect{ matcher.failure_message }.to_not raise_error

Choose a reason for hiding this comment

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

Space missing to the left of {.

@lime lime force-pushed the content-type-matcher-nil branch from dc6968f to e26f038 Compare June 29, 2015 07:12
@lime lime force-pushed the content-type-matcher-nil branch 3 times, most recently from 72cdafd to 719d293 Compare February 15, 2016 21:11
@lime
Copy link
Contributor Author

lime commented Feb 15, 2016

Could I maybe get some feedback on this?

I rebased the commits on top of the current master and pushed them one by one. This should show that the fix still applies, and that failure_message breaks without it.

@tute
Copy link
Contributor

tute commented May 9, 2016

This is great, @lime. Can you please rebase again on top of latest master? And please add a note on the change to the NEWS file, and we'll merge. Thank you! 😃

@lime
Copy link
Contributor Author

lime commented May 10, 2016

Thanks for the update! I'll take a look at it tonight. :)

lime added 2 commits May 24, 2016 23:49
…ssage

Note: this is most likely not the way we want to test our matchers, but it
demonstrates the current issue.
When a matcher uses both `allowing` and `rejecting`, and validation fails,
`@missing_rejected_types` might still be `nil`.

When a matcher only uses either of the two, a part of the message is `nil`.
@lime lime force-pushed the content-type-matcher-nil branch from b823c6f to 8cd76a8 Compare May 24, 2016 20:50
[ci skip]
@lime
Copy link
Contributor Author

lime commented May 24, 2016

Rebased, sorry for the delay.

@tute tute merged commit d61b706 into thoughtbot:master May 24, 2016
@tute
Copy link
Contributor

tute commented May 24, 2016

Thanks!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange Behaviour on validate_attachment_content_type
4 participants