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

Run actual stripe api tests on ci #1834

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

arnav13081994
Copy link
Collaborator

@arnav13081994 arnav13081994 commented Oct 26, 2022

This PR achieves the following:

  1. Updated 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.

@arnav13081994
Copy link
Collaborator Author

@jleclanche I do not have enough access to be able to Create Github Secrets.

@jleclanche jleclanche added this to the 2.8.0 milestone Dec 19, 2022
@arnav13081994 arnav13081994 force-pushed the feature/run_actual_stripe_api_tests_on_ci branch from 5d38d7b to 1e21d99 Compare February 14, 2023 05:30
@arnav13081994 arnav13081994 force-pushed the feature/run_actual_stripe_api_tests_on_ci branch from 1e21d99 to 8781256 Compare March 16, 2023 04:26
@jleclanche jleclanche force-pushed the feature/run_actual_stripe_api_tests_on_ci branch from 8781256 to a66f0fd Compare March 16, 2023 11:09
@arnav13081994 arnav13081994 force-pushed the feature/run_actual_stripe_api_tests_on_ci branch 5 times, most recently from a1266cb to 55715d9 Compare March 21, 2023 12:01


@pytest.fixture(autouse=True)
def slow_down_tests(request):
Copy link
Member

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

Copy link
Collaborator Author

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:

  1. All existing network calls need to be refactored to go through this universal djstripe's internal HTTP interface. Lets call it DJSTRIPEHTTPCLIENT.
  2. DJSTRIPEHTTPCLIENT must then handle stripe RateLimitError using exponential backoff up to a max number of retries.
  3. 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:

  1. Generating idempotency keys for every request.
    1. Providing the users with a mechanism of accessing the key so that they can also retry the request.
    2. But can also choose to generate a new one if they wish to do so.
  2. Updating the current code to pass idempotency key like stripe_account, api_key are passed around.
  3. Create DJSTRIPEHTTPCLIENT interface with exponential backoff etc.

Copy link
Member

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.

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Member

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.

@arnav13081994 arnav13081994 force-pushed the feature/run_actual_stripe_api_tests_on_ci branch from 462b61b to a77dfcb Compare March 22, 2023 06:05
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.
@arnav13081994 arnav13081994 force-pushed the feature/run_actual_stripe_api_tests_on_ci branch 7 times, most recently from edb2786 to f9bb4ec Compare March 30, 2023 11:08
@jleclanche jleclanche modified the milestones: 2.8.0, 3.0.0 Aug 29, 2023
@jleclanche jleclanche force-pushed the master branch 4 times, most recently from 15e04c3 to 8727894 Compare April 25, 2024 14:48
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.

None yet

2 participants