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

test(idempotency): add e2e tests for idempotency #1442

Merged
merged 17 commits into from
May 12, 2023
Merged

test(idempotency): add e2e tests for idempotency #1442

merged 17 commits into from
May 12, 2023

Conversation

am29d
Copy link
Contributor

@am29d am29d commented May 11, 2023

Description of your changes

This PR introduces e2e tests for idempotency using decorator and function wrapper. Several changes were also introduced to the interface and options type, here is the breakdown.

IdempotencyOptions

First, we have unwrapped the options object before passing it to the IdempotencyHandler, this helps to keep it generic and decide which parameters we should pass from decorator or wrapper. We also expanded the type to contain dataKeywordArgument and config as optional parameters. This enables us to apply decorator to lambda handler or any other method. In addition, we can use the same interface for decorator and wrapper.

Test cases

The test cases should help to test basic functionality. We have much more possible tests with permutations of input, config, persistence layer options and sdk options. Not all of them are covered yet and I suggest to use the beta phase to add more tests down the road. The current test cases are:

  • decorator
    • call method twice sequentially, the response value is in ddb
    • call method twice parallel, the response value is in ddb and we got one error during second call
    • call method once with custom persistence layer options
    • call method that fails, there is no ddb entry
    • call method with keyword argument with two different key/value pairs, the response values are in ddb
  • wrapper
    • call method twice sequentially, the response value is in ddb
    • call method with custom persistence layer options

Related issues, RFCs

Issue number: #447

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my change is effective and works
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@boring-cyborg boring-cyborg bot added idempotency This item relates to the Idempotency Utility dependencies Changes that touch dependencies, e.g. Dependabot, etc. tests PRs that add or change tests labels May 11, 2023
@pull-request-size pull-request-size bot added the size/XL PRs between 500-999 LOC, often PRs that grown with feedback label May 11, 2023
@am29d am29d requested a review from dreamorosi May 11, 2023 11:15
@dreamorosi dreamorosi changed the title tests(idempotency): add e2e tests for idempotency test(idempotency): add e2e tests for idempotency May 11, 2023
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR and for the hard work, I can see (and I know) you've put a lot of effort into this - I appreciate it.

Overall it's looking good - I have left some comments that I'd like to discuss before moving forward.

packages/idempotency/src/types/IdempotencyOptions.ts Outdated Show resolved Hide resolved
packages/idempotency/src/types/IdempotencyOptions.ts Outdated Show resolved Hide resolved
packages/idempotency/src/types/IdempotencyOptions.ts Outdated Show resolved Hide resolved
packages/idempotency/src/IdempotencyHandler.ts Outdated Show resolved Hide resolved
@dreamorosi
Copy link
Contributor

You can run npm run lint-fix -w packages/commons from the root dir to fix all the linting stuff that popped out. I didn't think it would affect your PR as I forgot the commons package was touched, my bad.

@dreamorosi dreamorosi self-requested a review May 12, 2023 17:19
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this, I've run the integration tests on this branch and it's passing.

I'm gonna fix the linting in a subsequent PR and also open an issue to remember ourselves to go back to the types once we are done with the main implementation.

Thank you again Alex, enjoy the weekend!

@dreamorosi dreamorosi merged commit 4ef4e5c into aws-powertools:main May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Changes that touch dependencies, e.g. Dependabot, etc. idempotency This item relates to the Idempotency Utility size/XL PRs between 500-999 LOC, often PRs that grown with feedback tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: write end-to-end tests for the utility
2 participants