amplify-cli icon indicating copy to clipboard operation
amplify-cli copied to clipboard

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

Open behrmans opened this issue 4 years ago • 6 comments

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

  • [X] PR description included
  • [X] yarn test passes
  • [X] 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 avatar Aug 13 '21 19:08 behrmans

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.

behrmans avatar Sep 05 '21 13:09 behrmans

Hey, any progress on it?

jettary avatar Nov 02 '21 13:11 jettary

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 :)

giraudvalentin avatar Dec 01 '21 15:12 giraudvalentin

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

behrmans avatar Jun 28 '22 16:06 behrmans

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%) :arrow_down:
...simulator/src/data-loader/dynamo-db/utils/index.ts 80.00% <100.00%> (+56.92%) :arrow_up:
...li/src/domain/amplify-usageData/getUsageDataUrl.ts 100.00% <0.00%> (+12.50%) :arrow_up:

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

codecov-commenter avatar Jun 28 '22 17:06 codecov-commenter

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

danielleadams avatar Jul 06 '22 15:07 danielleadams

Any news about this PR / draft ?

a-marcel avatar Jan 02 '23 21:01 a-marcel

@behrmans are you still working on this?

danielleadams avatar Jan 18 '23 20:01 danielleadams

Closing due to inactivity.

danielleadams avatar Apr 26 '23 21:04 danielleadams