Conversation
sbera87
force-pushed
the
cognito
branch
2 times, most recently
from
da489d7 to
b2a7581
Compare
sbera87
force-pushed
the
cognito
branch
3 times, most recently
from
88852d6 to
04683fc
Compare
| #include <aws/core/utils/Outcome.h> | ||
| #include <aws/testing/TestingEnvironment.h> | ||
| #include <aws/core/platform/Environment.h> | ||
| #include <openssl/sha.h> |
There was a problem hiding this comment.
why are we using openssl headers directly, this would create a openssl dependency for tests that we dont want to have. what do we need from openssl that we dont have in c-cal
| testTrace.erase(); | ||
| } | ||
|
|
||
| private: |
There was a problem hiding this comment.
nit: weird spacing on private
| } | ||
|
|
||
| private: | ||
| Aws::String createUserPool(Aws::String poolName) |
There was a problem hiding this comment.
lets use const Aws::String& since we arent calling move on it
| private: | ||
| Aws::String createUserPool(Aws::String poolName) | ||
| { | ||
| if(poolName.empty()) |
There was a problem hiding this comment.
shouldnt we never call with empty?
|
|
||
| CreateUserPoolOutcome userPoolOutcome = client->CreateUserPool(userPoolRequest); | ||
| if (!userPoolOutcome.IsSuccess()) { | ||
| return {}; |
There was a problem hiding this comment.
this is a test so we should expecting/asserting service outcomes with with either AWS_ASSERT_SUCCESS or AWS_EXPECT_SUCCESS
|
|
||
| auto deletePoolOutcome = client->DeleteUserPool(deletePoolRequest); | ||
| if (!deletePoolOutcome.IsSuccess()) { | ||
| AWS_LOGSTREAM_ERROR(ALLOCATION_TAG, "Failed to delete User Pool: " << deletePoolOutcome.GetError().GetMessage()); |
There was a problem hiding this comment.
yeah last comment ill leave on this we shoudlnt be logging errors, this is a test therefore we want to assert/expecting
| } | ||
| ~SRPClient() | ||
| { | ||
| BN_free(N); |
There was a problem hiding this comment.
yeah we cant be using linux/openssl symbols otherwise this wont work on windows. also we dont want to make the tests depedent on being run in linux or a direct open ssl depedency.
For crypto try to only use our utility classes, which invokes aws-c-cal instead of taking on a direct crypto dependency.
any crypto related functionality should be flowing through the CRT
2 participants
Issue #, if available:
Description of changes:
Add Integration tests for cognito-idp
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.