-
-
Notifications
You must be signed in to change notification settings - Fork 476
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
Run actual stripe api tests on ci #1834
base: master
Are you sure you want to change the base?
Conversation
@jleclanche I do not have enough access to be able to Create Github Secrets. |
5d38d7b
to
1e21d99
Compare
1e21d99
to
8781256
Compare
8781256
to
a66f0fd
Compare
a1266cb
to
55715d9
Compare
|
||
|
||
@pytest.fixture(autouse=True) | ||
def slow_down_tests(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arnav13081994 this isn't the right way to do it ... if you run into 429s the tests should know to retry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jleclanche That was my first approach to fixing this issue. The following complicates the implementation:
- All existing network calls need to be refactored to go through this universal djstripe's internal HTTP interface. Lets call it
DJSTRIPEHTTPCLIENT
. DJSTRIPEHTTPCLIENT
must then handle stripeRateLimitError
usingexponential
backoff up to a max number of retries.- So far everything is well and good till you realise that we can't retry stripe requests without an
idempotency
key for risk of creating multiple objects.
Therefore the only way we can handle this is by:
- Generating
idempotency
keys for every request.- Providing the users with a mechanism of accessing the key so that they can also retry the request.
- But can also choose to generate a new one if they wish to do so.
- Updating the current code to pass
idempotency
key likestripe_account
,api_key
are passed around. - Create
DJSTRIPEHTTPCLIENT
interface with exponential backoff etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, this retry strategy with idempotency key is something that should be in dj-stripe core. We have the tools for it. I think your tests are essentially showcasing the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and was planning on adding DJSTRIPEHTTPCLIENT
to the codebase anyway but this wait
solution seems like a good enough fix (for now) just to be able to start adding tests using the stripe api
. I can probably try to put together the DJSTRIPEHTTPCLIENT
over the weekend but that would need to be very, very thoroughly tested as this would be a huge change because of potential duplicate objects getting created in production.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's just to land the tests then ok, but I worry about it severely increasing the run time of our CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Is there anyway we can review Github Actions usage? It seems I do not have enough access.
Please check it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't limited in usage but I want to keep it to a minimum anyway. It's wasteful in energy and it increases iteration time on PRs.
462b61b
to
a77dfcb
Compare
This was done in order for the STRIPE_TEST_SECRET_KEY env var to be passed from tox to pytest.
This was done in order to get around RateLimit and other related errors.
This was done because tests would fail due to low coverage of tests using the api. This is a temp fix until more tests can be added.
edb2786
to
f9bb4ec
Compare
15e04c3
to
8727894
Compare
This PR achieves the following:
ci.yml
to run tests using the Actual Stripe API injected through Github Secrets.Note: This PR should only be merged after #1832 gets merged.