slack-ruby / slack-ruby-bot

The easiest way to write a Slack bot in Ruby.
MIT License
1.12k stars 187 forks source link

Consider splitting RSpec shared behaviours and development dependencies #257

Closed dikond closed 4 years ago

dikond commented 4 years ago

Hola :v:

First of all thanks for your awesome project! We're happily using it for at least a year now in production. But today I spotted a minor problem. We're requiring slack-ruby-bot/rspec in spec_helper (as it's noted in the README) which loads and configures VCR gem and it causing some unexpected exceptions to be raised by VCR, e.g.

================================================================================
An HTTP request has been made that VCR does not know how to handle:
  GET https://www.iana.org/domains/reserved?foo=moo

There is currently no cassette in use. There are a few ways
you can configure VCR to handle this request:

  * If you're surprised VCR is raising this error
    and want insight about how VCR attempted to handle the request,
    you can use the debug_logger configuration option to log more details [1].
  * If you want VCR to record this request and play it back during future test
    runs, you should wrap your test (or this portion of your test) in a
    `VCR.use_cassette` block [2].
  * If you only want VCR to handle requests made while a cassette is in use,
    configure `allow_http_connections_when_no_cassette = true`. VCR will
    ignore this request since it is made when there is no cassette [3].
  * If you want VCR to ignore this request (and others like it), you can
    set an `ignore_request` callback [4].

[1] https://www.relishapp.com/vcr/vcr/v/4-0-0/docs/configuration/debug-logging
[2] https://www.relishapp.com/vcr/vcr/v/4-0-0/docs/getting-started
[3] https://www.relishapp.com/vcr/vcr/v/4-0-0/docs/configuration/allow-http-connections-when-no-cassette
[4] https://www.relishapp.com/vcr/vcr/v/4-0-0/docs/configuration/ignore-request
================================================================================

I'll add a few notes about our project setup:

You may see where it leads already. The exception above were a surprise because we don't even load our VCR helper with configuration in a given spec. We haven't spotted this problem immediately on slack-ruby-bot/rspec addition because most of the time we're running the whole suite and due to a load order our VCR configuration usually loaded shortly after slack-ruby-bot/rspec/support/vcr and overrides the config.

I appreciate the effort to ship RSpec matchers and shared behaviour with this gem but these things do not require VCR as a dependency (I've checked how VCR is used).

I'd like to open up minimal PR to fix the issue, by excluding support/vcr from eager loading and require it only in the gem specs that need it. That's the cheapest way to fix the issue I guess. However, I think that such eager-loading may cause further problems of similar nature in the future. Ideally, I'd create a separate gem for RSpec shared matches and behaviours to physically split development dependencies and public API.

dblock commented 4 years ago

I would support a separate slack-ruby-bot-rspec gem, however that's not going to fix your problem if it were identical to what we have here and you required it. I suppose it would split development dependencies properly and so this wouldn't be an issue? Sounds like that's the fix here too.

So what you suggest sounds good to me. Without digging into it none of the shared matchers need VCR, or do they? Refactor those out.

dikond commented 4 years ago

none of the shared matchers need VCR

That's correct. I'll prepare a PR a bit later then.

dikond commented 4 years ago

@dblock may I ask you when do you plan to publish a new gem version?

dblock commented 4 years ago

@dblock may I ask you when do you plan to publish a new gem version?

Done.