Change to resolve error when url is 'nil'

[dmvt]

When running tests with RSpec & Capybara we (Punchpass) were encountering the following error:

Failures:

  1) Customer signs in via the company sign in page having a customer account with the company with an instance param is taken to the member instance details page
     Failure/Error: - breadcrumb_trail crumb_length: 60 do |name, url, styles|

     ActionView::Template::Error:
       arguments passed to url_for can't be handled. Please require routes or provide your own implementation
     # /Users/dan/.asdf/installs/ruby/2.4.2/lib/ruby/gems/2.4.0/gems/actionview-4.2.8/lib/action_view/helpers/url_helper.rb:38:in `url_for'
     # /Users/dan/.asdf/installs/ruby/2.4.2/lib/ruby/gems/2.4.0/gems/loaf-0.6.0/lib/loaf/view_extensions.rb:55:in `block in breadcrumb_trail'
     # /Users/dan/.asdf/installs/ruby/2.4.2/lib/ruby/gems/2.4.0/gems/loaf-0.6.0/lib/loaf/view_extensions.rb:53:in `each'
     # /Users/dan/.asdf/installs/ruby/2.4.2/lib/ruby/gems/2.4.0/gems/loaf-0.6.0/lib/loaf/view_extensions.rb:53:in `breadcrumb_trail'
     # ./app/views/layouts/member_area.html.slim:50:in `_app_views_layouts_member_area_html_slim__403637215859839844_70256255168280'

After a bit of digging, I found the cause to be a nil value in the url for our last breadcrumb. Interestingly, the error didn't trip in development or production, just testing. Adding the nil check allowed our tests to pass gracefully.

[coveralls]

Coverage remained the same at 97.03% when pulling 36bbffb538127d54a7051e4d2866911b7da606bd on dmvt:master into c7f7468f9f056c6bdd91767f2c4ab1e332ea9ad5 on piotrmurach:master.

[coveralls]

Coverage remained the same at 97.03% when pulling 36bbffb538127d54a7051e4d2866911b7da606bd on dmvt:master into c7f7468f9f056c6bdd91767f2c4ab1e332ea9ad5 on piotrmurach:master.

[coveralls]

Coverage remained the same at 97.03% when pulling 36bbffb538127d54a7051e4d2866911b7da606bd on dmvt:master into c7f7468f9f056c6bdd91767f2c4ab1e332ea9ad5 on piotrmurach:master.

[coveralls]

Coverage remained the same at 97.03% when pulling 36bbffb538127d54a7051e4d2866911b7da606bd on dmvt:master into c7f7468f9f056c6bdd91767f2c4ab1e332ea9ad5 on piotrmurach:master.

[coveralls]

Coverage remained the same at 97.03% when pulling 36bbffb538127d54a7051e4d2866911b7da606bd on dmvt:master into c7f7468f9f056c6bdd91767f2c4ab1e332ea9ad5 on piotrmurach:master.

[coveralls]

Coverage remained the same at 97.03% when pulling 36bbffb538127d54a7051e4d2866911b7da606bd on dmvt:master into c7f7468f9f056c6bdd91767f2c4ab1e332ea9ad5 on piotrmurach:master.

[dmvt]

Another potential solution to this would be to check for nil in the url at the breadcrumb creation time and present an error. If you'd like me to redo this using that approach, I'm happy to.

[piotrmurach]

Hi Dan,

Thanks for using the library!

I don't fully agree with your reasoning. If I understand correctly, you have an issue running your tests as one of the crumbs has nil value for url param. I don't believe this library should be checking for such cases. Firstly, creating or having breadcrumbs with nil links is an application level erroneous condition and that's where I believe it should be fixed. What if a breadcrumb is created without any name, should the library check for that as well? Most importantly, your solution decides that nil value should be substituted for some other string value which may lead to other bugs. Having said that, I would be happy to raise an error during crumb initialization to ensure consistent data.