search

rikas / zoho_hub

Zoho CRM API V2 Wrapper
MIT License
25 stars 30 forks source link

Removes vcr in favor of plain webmock #62

Closed nwittstruck closed 3 years ago

nwittstruck commented 3 years ago

Hi @rikas , I've noticed that rspec is currently broken, unless certain ENV vars are set, see #60. This also broke CI. I've fixed this and made sure that the specs run without ENV vars. While doing so, I went ahead and replaced the VCR example with plain webmock. I hope this is ok: 1) It's very easy to miss auth tokens in cassette files. In this case, they would end up in a public repo. 2) I prefer to make the expected requests explicit, otherwise it's quite easy to miss what's going on in the background.

If you prefer VCR, that's fine, I'll put it back in and just fix the failing spec. We could also discuss to remove the ENV vars for testing completely, as changing them might break existing specs (i.e. VCR / Webmock wouldn't be able to catch the requests, since the url and authentication parameters change when changing the connection vars).

This should unblock a couple of pull request, so it would be great if we could merge this in. If you're fine with switching to plain webmock, I'll add more specs.

Afterwards, I also suggest to switch to Github Actions instead of TravisCI, since they've changed their support for open source projects. I can do this, if you'd like. Just let me know. Thanks!

nwittstruck commented 3 years ago

The build is failing because of some rubocop warnings in untouched code. I guess they were introduced since the CI uses a more recent version of rubocop. Do you want me to fix them in this PR or a separate PR?

alexcwatt commented 3 years ago

@rikas I am looking to use ZohoHub with Ruby 3.0.1 and opened PR #64 for that. I couldn't run rspec locally for the same reason as @nwittstruck. Is there anything we can do to merge this PR? I am happy to help if you would like more changes, e.g. Rubocop compliance. (Also in my PR I fixed a few things to get Rubocop passing.)

rikas commented 3 years ago

Hi all, sorry, I've been super busy lately. This seems like a reasonable PR, but I see that we can merge #65, that already includes this, right?

I'm closing this one in favor of #65