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

Stripe::Charge.list difficult to test in rspec 3.8 #687

Closed
KelseyDH opened this issue Sep 13, 2018 · 3 comments
Closed

Stripe::Charge.list difficult to test in rspec 3.8 #687

KelseyDH opened this issue Sep 13, 2018 · 3 comments

Comments

@KelseyDH
Copy link

KelseyDH commented Sep 13, 2018

Subject of the issue

I have a working rspec test for Stripe::Charge.list that works in rspec 3.7.1 but breaks in rspec 3.8.x. The maintainers of rspec say this is due to a problem with how the Stripe gem defines #hash on its objects.

Your environment

  • Ruby version: 2.5.1
  • rspec-expectations version: 3.8.1
  • stripe 3.5.3

Steps to reproduce

The Stripe::Charge.list object id changes every time it is called, even if the data it gives back is the same. Meaning a call like this now fails:

            it "does not create a charge in stripe" do
              expect{ true }.to_not change{ Stripe::Charge.list }
            end

Expected behavior

The expected behaviour is that calls to two Stripe::Charge.list objects should consider not changed if the attributes within that object remain unchanged.

E.g.:

#<Stripe::ListObject:0x3feae3af47b4> JSON: {
  "object": "list",
  "data": [
    {"id":"test_ch_6","object":"charge","created":1366194027,"livemode":false,"paid":true,"amount":34800,"application_fee":null,"currency":"usd","destination":null,"fraud_details":{},"receipt_email":null,"receipt_number":null,"refunded":false,"shipping":{},"statement_descriptor":"Charge test_ch_6","status":"succeeded","source":{"object":"card","last4":"4242","type":"Visa","brand":"Visa","funding":"credit","exp_month":12,"exp_year":2013,"fingerprint":"3TQGpK9JoY1GgXPw","country":"US","name":"name","address_line1":null,"address_line2":null,"address_city":null,"address_state":null,"address_zip":null,"address_country":null,"cvc_check":null,"address_line1_check":null,"address_zip_check":null},"captured":true,"refunds":{"object":"list","total_count":0,"has_more":false,"url":"/v1/charges/test_ch_6/refunds","data":[]},"transfer":null,"balance_transaction":"test_txn_1","failure_message":null,"failure_code":null,"amount_refunded":0,"customer":{"id":"test_cus_3","email":"[email protected]","description":"an auto-generated stripe customer data mock","object":"customer","created":1372126710,"livemode":false,"delinquent":false,"discount":null,"account_balance":0,"currency":null,"sources":{"object":"list","total_count":1,"url":"/v1/customers/test_cus_3/sources","data":[{"id":"test_cc_2","object":"card","last4":"9191","type":"Visa","brand":"Visa","funding":"credit","exp_month":4,"exp_year":2018,"fingerprint":"wXWJT135mEK107G8","customer":"test_cus_3","country":"US","name":"Johnny App","address_line1":null,"address_line2":null,"address_city":null,"address_state":null,"address_zip":null,"address_country":null,"cvc_check":null,"address_line1_check":null,"address_zip_check":null,"tokenization_method":null,"number":null}]},"subscriptions":{"object":"list","total_count":0,"url":"/v1/customers/test_cus_3/subscriptions","data":[]},"default_source":"test_cc_2","metadata":{}},"invoice":null,"description":null,"dispute":null,"metadata":{}}
  ],
  "url": "/v1/hashs",
  "has_more": false
}

versus

#<Stripe::ListObject:0x3feae276ca0c> JSON: {
  "object": "list",
  "data": [
    {"id":"test_ch_6","object":"charge","created":1366194027,"livemode":false,"paid":true,"amount":34800,"application_fee":null,"currency":"usd","destination":null,"fraud_details":{},"receipt_email":null,"receipt_number":null,"refunded":false,"shipping":{},"statement_descriptor":"Charge test_ch_6","status":"succeeded","source":{"object":"card","last4":"4242","type":"Visa","brand":"Visa","funding":"credit","exp_month":12,"exp_year":2013,"fingerprint":"3TQGpK9JoY1GgXPw","country":"US","name":"name","address_line1":null,"address_line2":null,"address_city":null,"address_state":null,"address_zip":null,"address_country":null,"cvc_check":null,"address_line1_check":null,"address_zip_check":null},"captured":true,"refunds":{"object":"list","total_count":0,"has_more":false,"url":"/v1/charges/test_ch_6/refunds","data":[]},"transfer":null,"balance_transaction":"test_txn_1","failure_message":null,"failure_code":null,"amount_refunded":0,"customer":{"id":"test_cus_3","email":"[email protected]","description":"an auto-generated stripe customer data mock","object":"customer","created":1372126710,"livemode":false,"delinquent":false,"discount":null,"account_balance":0,"currency":null,"sources":{"object":"list","total_count":1,"url":"/v1/customers/test_cus_3/sources","data":[{"id":"test_cc_2","object":"card","last4":"9191","type":"Visa","brand":"Visa","funding":"credit","exp_month":4,"exp_year":2018,"fingerprint":"wXWJT135mEK107G8","customer":"test_cus_3","country":"US","name":"Johnny App","address_line1":null,"address_line2":null,"address_city":null,"address_state":null,"address_zip":null,"address_country":null,"cvc_check":null,"address_line1_check":null,"address_zip_check":null,"tokenization_method":null,"number":null}]},"subscriptions":{"object":"list","total_count":0,"url":"/v1/customers/test_cus_3/subscriptions","data":[]},"default_source":"test_cc_2","metadata":{}},"invoice":null,"description":null,"dispute":null,"metadata":{}}
  ],
  "url": "/v1/hashs",
  "has_more": false
}

Actual behavior

Instead the actual behaviour is that the .change matcher on 3.8 now detects this new object ID as a change when in 3.7.x it didn't.


I brought this problem up with rpec's maintainers, and they stated it's an issue that should be fixed within the Stripe gem itself. Quote:

"the best fix here would be to get a change into the stripe gem so that they do define #hash on these objects properly."

brandur-stripe pushed a commit that referenced this issue Sep 13, 2018
…quivalency

Overrides `#eql?` (hash equality) and `#hash` so that Stripe objects can
be used more easily as Hash keys and that certain other frameworks that
rely on these methods will have an easier time (e.g. RSpec's `change`,
see #687).

I think this might be a little controversial if we weren't already
overriding the `#==` implementation, but because we are, I think it
makes sense to extent it to these two methods as well.
@brandur-stripe
Copy link
Contributor

brandur-stripe commented Sep 13, 2018

Alright, we're already overriding ==, so I think it makes sense to extend that equality to eql? and hash. Calling this a bug (i.e., as implied in that RSpec thread) isn't right though as no Ruby docs imply a requirement for classes to be hash key friendly, and two separate calls to .list are clearly going to produce a different set of objects. Opened a PR in #688.

BTW, I'd personally be a little cautious with this testing approach. By making checks dependent on list you're introducing an element of slowness and an element of brittleness from those extra network requests. You might consider mocking either via RSpec or webmock to check that no request to the Stripe API occurred.

@ob-stripe
Copy link
Contributor

Fixed in 3.26.1.

@KelseyDH
Copy link
Author

@ob-stripe @brandur-stripe Thanks for the lightning fast response!

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

No branches or pull requests

3 participants