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

Fix request headers in tests #374

Merged
merged 1 commit into from
Aug 25, 2017
Merged

Conversation

jlomas-stripe
Copy link
Contributor

My tests were bad and I should feel bad.

I was sending api_version in the header, instead of stripe_version, so it ... wasn't actually setting them.

Couldn't understand why the versions needed updating in #371 but after investigating I realized it's because I wasn't actually specifying them, and latest no longer mapped to the value I was using.

r? @brandur-stripe

@brandur-stripe
Copy link
Contributor

Haha, I guess this is one the downfall of unit tests — you're testing what you already expect to occur :) Happens to all of us though.

How can you just change this without updating any mainline code though? Has the build been broken this whole time, or are the tests not actually checking anything?

@jlomas-stripe
Copy link
Contributor Author

jlomas-stripe commented Aug 22, 2017

It broke when Stripe released 2017-08-15, so I think it's simply a result of nothing having triggered a build since before 2017-08-15 was released.

That said, the tests were 'magically working' because latest happened to equal the hard-coded version I'd thought I was passing through (but was not); once latest meant something different, it broke, so now I'm actually passing through the right parameter.

@brandur-stripe
Copy link
Contributor

Nice! Okay, I think I misunderstood what we were changing initially, but totally makes sense now. Thanks!

@brandur-stripe brandur-stripe merged commit 375d9e3 into master Aug 25, 2017
@brandur-stripe brandur-stripe deleted the jloms-fix-event-tests branch August 25, 2017 19:42
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

Successfully merging this pull request may close these issues.

2 participants