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

feat: add BatchGetItem to appsync-simulator(#5963) #7952

Closed

Conversation

behrmans
Copy link

Description of changes

  • add BatchGetItem to amplify-appsync-simulator (all credit to tan-t)
  • Added reference to path amplify-e2e-core in amplify-migration-tests package due to errors regarding amplify-e2e-core not being found.

Issue #, if available

Issue 5963
Copied almost all code from PR 5965
Reason I made new PR was because it seemed like a lot of time had passed (9 months) since the original PR had made progress on the failing test. Please let me know if I should have handled this differently and I will correct any issues ASAP

Description of how you validated changes

  • ran existing unit tests
  • ran unit test for added utility function
  • done E2E manual tests and checked that the BatchGetItem operation will returns response like documented

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@behrmans behrmans requested a review from a team as a code owner August 13, 2021 19:03
CurtisHughes
CurtisHughes previously approved these changes Aug 13, 2021
@siegerts siegerts added the first-time-contributor The contribution is the first for this user in the repo label Aug 17, 2021
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Can you please add E2E tests.

@@ -12,5 +12,8 @@
"declaration": true,
"typeRoots": ["../../node_modules/@types", "node_modules/@types", "./typings"]
},
"references": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an unrelated change. Can you remove it.

@behrmans
Copy link
Author

behrmans commented Sep 5, 2021

Can you please add E2E tests.

Is there a place where I should put them? I'm trying to figure out where the e2e tests for the other operations have been placed. For now, I'm working on adding to amplify-util-mock but can move it somewhere else if needed.

jettary
jettary previously approved these changes Sep 10, 2021
@jettary
Copy link

jettary commented Nov 2, 2021

Hey, any progress on it?

@giraudvalentin
Copy link

Hi, same question, because i changed my offline appsync plugin to use this because its "active & maintened" but face this problem too.

To bypass the problem, i have builded the package amplify-appsync-simulator with your code, used npm i && cd package/amplify-simulator && yarn build and commited the change to a repository

Then, i can use it in the serverless-appsync-simulator as dependancies (in a fork too)

Finally, i use this fork in my serverless project !

Thank you very much @behrmans & others :)

@behrmans
Copy link
Author

Reason for force-push was branch was so far behind and had trouble rebasing. All seems fine now. I still plan on trying to finish this PR hopefully soon

@codecov-commenter
Copy link

Codecov Report

Merging #7952 (62bf704) into master (4084827) will increase coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #7952      +/-   ##
==========================================
+ Coverage   47.29%   47.31%   +0.01%     
==========================================
  Files         665      665              
  Lines       32931    32942      +11     
  Branches     6641     6645       +4     
==========================================
+ Hits        15576    15587      +11     
  Misses      15682    15682              
  Partials     1673     1673              
Impacted Files Coverage Δ
...psync-simulator/src/data-loader/dynamo-db/index.ts 11.65% <20.00%> (-0.06%) ⬇️
...simulator/src/data-loader/dynamo-db/utils/index.ts 80.00% <100.00%> (+56.92%) ⬆️
...li/src/domain/amplify-usageData/getUsageDataUrl.ts 100.00% <0.00%> (+12.50%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@danielleadams
Copy link
Member

@behrmans can we move this to draft since its still a work in progress? Or are you ready for review?

@evcodes evcodes marked this pull request as draft December 1, 2022 18:09
@a-marcel
Copy link

a-marcel commented Jan 2, 2023

Any news about this PR / draft ?

@danielleadams
Copy link
Member

@behrmans are you still working on this?

@danielleadams danielleadams added the pending-response Issue is pending response from the issue author label Jan 18, 2023
@danielleadams
Copy link
Member

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time-contributor The contribution is the first for this user in the repo pending-response Issue is pending response from the issue author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants