refactor(amplify_auth_cognito): interface changes, logs, tests #27

haverchuck · 2020-08-13T17:16:12Z

Description of changes:
This PR introduces interface changes, flutter sdk version bump, platform logging, and android unit tests

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

…mp and android unit tests

packages/amplify_auth_cognito/ios/Classes/SwiftAuthCognito.swift

...d/src/test/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AmplifyAuthCognitoPluginTest.kt

packages/amplify_auth_cognito/android/build.gradle

...th_cognito/android/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AuthCognito.kt

packages/amplify_auth_cognito/ios/Classes/SwiftAuthCognito.swift

...d/src/test/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AmplifyAuthCognitoPluginTest.kt

jamesonwilliams · 2020-08-14T01:43:53Z

...d/src/test/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AmplifyAuthCognitoPluginTest.kt

+        doAnswer { invocation: InvocationOnMock ->
+            plugin.prepareSignUpResult(mockResult, mockSignUpResult)
+            null as Void?
+        }.`when`(mockAuth).confirmSignUp(anyString(), anyString(), ArgumentMatchers.any<Consumer<AuthSignUpResult>>(), ArgumentMatchers.any<Consumer<AuthException>>())
+
+        doAnswer { invocation: InvocationOnMock ->
+            plugin.prepareSignUpResult(mockResult, mockSignUpResult)
+            null as Void?
+        }.`when`(mockAuth).resendSignUpCode(anyString(), ArgumentMatchers.any<Consumer<AuthSignUpResult>>(), ArgumentMatchers.any<Consumer<AuthException>>())


Instead of making all of these arrangements in the single seutp call, split these down into the individual @Test.

Each test should read like:

// Arrange doAnswer { ... // Act: callYourChannelBindingStuff(); // Assert: verify( /* a call was made on the amplify android mock object */);

One more note: it looks like mockAuth is an AuthCategory instance -- like Amplify Android's AuthCategory, right? While it may be technically possible to mock that, I would not. I would either:

Use an AuthCategoryBehavior in your source code, which is an interface. Then, use a mock(AuthCategoryBeahvior::class.java) in your test code, to run mockito verify(...) calls on.

**Use a real AuthCategory(), and do authCateogory.addPlugin(mockPlugin), where mockPlugin is a mock(AuthPlugin::class.java). Reason being: the AuthCategory is not designed to be extended, in fact I think it's a final class isnt' it? Whereas the AuthCategoryBehavior and AuthPlugin are deliberately designed to be extended, so creating a mock extension fits 100% with the source design.

I need to speak with the team about this larger change with the mocks, but the general organization suggestions you've made here I've completed.

That makes sense, yea! Also, I believe you can factor away from the Amplify.StaticGizmo calls in a non-breaking way, after release.

By playing one of these games:

final class SomeClass { private final GizmoBehaviorInterface gizmo; // This is used in source code. SomeClass() { this(Amplify.StaticGizmo); } // This is used in test code. SomeClass(GizmoBehaviorInterface gizmo) { this.gizmo = gizmo; } }

jamesonwilliams · 2020-08-14T01:45:12Z

...d/src/test/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AmplifyAuthCognitoPluginTest.kt

+import java.lang.Exception
+import java.lang.reflect.Field
+import java.lang.reflect.Modifier


Get rid of the reflection stuff. Don't set field state or anything like that. Respect the final keyword on our classes. I discuss approaches to ingest mock objects, below.

I need to talk to the team about this, not gonna make this change right now. It'll impact other tests.

…mp and android unit tests

…lutter into unitTestsAndLogging

skywardpixel

Left two comments on gradle configs. I'll leave the other changes for @fjnoyp and @Ashish-Nanda to review.

packages/amplify_auth_cognito/android/build.gradle

skywardpixel · 2020-08-15T01:14:33Z

packages/amplify_core/android/gradle/wrapper/gradle-wrapper.properties

@@ -2,4 +2,4 @@ distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.2-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-6.1.11-all.zip


Did you mean 6.1.1? 6.1.11 doesn't seem to exist.

the error in circle ci told me to use this version

I would just use the latest version, which right now is 6.6. Amplify Android just always uses the latest stable version.

README.md

...th_cognito/android/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AuthCognito.kt

packages/amplify_auth_cognito/ios/Classes/SwiftAuthCognito.swift

fjnoyp · 2020-08-15T01:55:50Z

packages/amplify_auth_cognito/ios/Classes/SwiftAuthCognito.swift

+          let errorCode = error != nil ? "\(error!)" : "signedOut"
+          logErrorContents(messages: [localizedError, recoverySuggestion, errorCode])
+          formatError(flutterResult: flutterResult, errorCode: errorCode, msg: msg, localizedError: localizedError, recoverySuggestion: recoverySuggestion)
        }
        if case .sessionExpired(let localizedError, let recoverySuggestion, let error) = error {
-             let errorCode = error != nil ? "\(error!)" : "sessionExpired"
-              formatError(flutterResult: flutterResult, errorCode: errorCode, msg: msg, localizedError: localizedError, recoverySuggestion: recoverySuggestion)
+          let errorCode = error != nil ? "\(error!)" : "sessionExpired"
+          logErrorContents(messages: [localizedError, recoverySuggestion, errorCode])
+          formatError(flutterResult: flutterResult, errorCode: errorCode, msg: msg, localizedError: localizedError, recoverySuggestion: recoverySuggestion)


You can call localizedError and recoverySuggestion on the underlying error right?

So can you do something like this:

var localizedError = error.localizedError;
var recoverySuggestion = error.recoverySuggestion;
var errorCode;
var messages = [ localizedError, recoverySuggestion ]

if case .signedOut(let localizedError, let recoverySuggestion, let error) = error {
errorCode = " ..... " : "SignedOut";
messages.Append(errorCode)
}
if case .sessionExpired(let localizedError, let recoverySuggestion, let error) = error {
...
}
....

logErrorContents(messages: messages)
formatError(flutterResult: ....... recoverySuggestion: recoverySuggestion)

jamesonwilliams

Nice!

jamesonwilliams · 2020-08-15T03:16:40Z

...roid/src/main/kotlin/com/amazonaws/amplify/amplify_auth_cognito/types/FlutterSignUpResult.kt

@@ -18,21 +18,25 @@ package com.amazonaws.amplify.amplify_auth_cognito.types
 import com.amplifyframework.auth.result.AuthSignUpResult
 import com.google.gson.Gson

-data class FlutterSignUpResult(private val raw: AuthSignUpResult) {
+data class FlutterSignUpResult(private var raw: AuthSignUpResult) {


can you keep it as val?

jamesonwilliams · 2020-08-15T03:16:51Z

...d/src/test/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AmplifyAuthCognitoPluginTest.kt

@@ -0,0 +1,303 @@
+package com.amazonaws.amplify.amplify_auth_cognito


Suggested change

package com.amazonaws.amplify.amplify_auth_cognito

package com.amazonaws.amplify.amplify_auth_cognito

jamesonwilliams · 2020-08-15T03:18:41Z

packages/amplify_core/android/gradle/wrapper/gradle-wrapper.properties

@@ -2,4 +2,4 @@ distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.2-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-6.1.11-all.zip


I would just use the latest version, which right now is 6.6. Amplify Android just always uses the latest stable version.

haverchuck added 4 commits August 13, 2020 10:05

refactor(amplify_auth_cognito): interface changes, flutter version bu…

29b0a91

…mp and android unit tests

remove test code

53e4879

setting log level

866b082

fixing dart tests

702e429

haverchuck requested review from fjnoyp and Ashish-Nanda August 13, 2020 18:13

fjnoyp reviewed Aug 13, 2020

View reviewed changes

packages/amplify_auth_cognito/ios/Classes/SwiftAuthCognito.swift Outdated Show resolved Hide resolved

fixing typo, removing dead test

4c170bb

fjnoyp approved these changes Aug 13, 2020

View reviewed changes

...d/src/test/kotlin/com/amazonaws/amplify/amplify_auth_cognito/AmplifyAuthCognitoPluginTest.kt Show resolved Hide resolved

jamesonwilliams reviewed Aug 14, 2020

View reviewed changes

jamesonwilliams mentioned this pull request Aug 14, 2020

chore: update android tool and dependency versions #28

Closed

mlabieniec added 3 commits August 14, 2020 12:43

Update README.md

948110d

Update README.md

eb65a0e

Update README.md

df34b80

skywardpixel mentioned this pull request Aug 14, 2020

test: refactor unit test for amplify_analytics_pinpoint #29

Closed

haverchuck added 13 commits August 14, 2020 13:09

bug fixes

ca87829

bug fixes

8b7f5e9

pr suggestions

9738a6e

refactor(amplify_auth_cognito): interface changes, flutter version bu…

d92f279

…mp and android unit tests

remove test code

e75e413

fixing dart tests

45b7065

fixing typo, removing dead test

71295c3

bug fixes

810d3fb

bug fixes

75aeaa4

pr suggestions

0c042a8

Merge branch 'unitTestsAndLogging' of github.com:haverchuck/amplify-f…

3d8d307

…lutter into unitTestsAndLogging

core gradle properties

794d94b

example build gradle fix

66c03ac

skywardpixel reviewed Aug 15, 2020

View reviewed changes

haverchuck added 2 commits August 14, 2020 18:33

gradle experiment

84fb9ca

gradle experiment

4ea8900

fjnoyp reviewed Aug 15, 2020

View reviewed changes

adding error log

04d7de1

jamesonwilliams approved these changes Aug 15, 2020

View reviewed changes

fjnoyp approved these changes Aug 15, 2020

View reviewed changes

haverchuck merged commit a0a1ed7 into aws-amplify:master Aug 15, 2020

magicmass mentioned this pull request Aug 21, 2020

No implementation found for method configure on channel com.amazonaws.amplify/core #80

Closed

aymane6 mentioned this pull request Nov 20, 2020

is Amplify analytics mandatory ? Because i can't create project with auth only #202

Closed

Abdelazeem777 mentioned this pull request Dec 22, 2021

Query predicates do not properly support lists for observeQuery() #1229

Closed

stam0912 mentioned this pull request Dec 2, 2023

App built with Xcode 15 crashes on iOS less than 17 with EXC_BAD_ACCESS when using the Networking framework #4195

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(amplify_auth_cognito): interface changes, logs, tests #27

refactor(amplify_auth_cognito): interface changes, logs, tests #27

haverchuck commented Aug 13, 2020

jamesonwilliams Aug 14, 2020

haverchuck Aug 14, 2020

jamesonwilliams Aug 15, 2020 •

edited

Loading

jamesonwilliams Aug 14, 2020

haverchuck Aug 14, 2020

skywardpixel left a comment •

edited

Loading

skywardpixel Aug 15, 2020

haverchuck Aug 15, 2020

jamesonwilliams Aug 15, 2020

fjnoyp Aug 15, 2020

jamesonwilliams left a comment

jamesonwilliams Aug 15, 2020

jamesonwilliams Aug 15, 2020

jamesonwilliams Aug 15, 2020

		@@ -0,0 +1,303 @@
		package com.amazonaws.amplify.amplify_auth_cognito

refactor(amplify_auth_cognito): interface changes, logs, tests #27

refactor(amplify_auth_cognito): interface changes, logs, tests #27

Conversation

haverchuck commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesonwilliams Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skywardpixel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesonwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesonwilliams Aug 15, 2020 •

edited

Loading

skywardpixel left a comment •

edited

Loading