chore(authenticator_test): Updated Golden tests to run on any screen #3655

Equartey · 2023-08-30T20:45:20Z

Description of changes:

Updated Golden tests to run on any screen & expands the golden test suite to test every screen, except for AuthenticatorStep.loading.
Fixed Beta channel failures
Golden test failures are now saved as artifacts for download
amplify_authenticator.ymal workflow uses a MacOS runner to ensure more consistent testing

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

dnys1 · 2023-09-05T16:42:50Z

packages/authenticator/amplify_authenticator/pubspec.yaml

+
+aft:
+  github:
+    custom: true


Not sure this is the best way to handle this. It means taking responsibility for updating the authenticator workflow file for any changes in the future. And the goldens thing may pop up in other packages if we add UI components. Can we handle this in generate_workflows instead? Basically, add a check for the existence of goldens and make flutter_vm.yaml take a boolean flag for whether there are goldens or not?

dnys1 · 2023-09-05T16:42:59Z

packages/authenticator/amplify_authenticator/lib/amplify_authenticator.dart

@@ -437,6 +438,9 @@ class Authenticator extends StatefulWidget {
  /// {@macro amplify_authenticator_dial_code_options}
  final DialCodeOptions dialCodeOptions;

+  @visibleForTesting
+  final AuthenticatorState? mockAuthenticatorState;


Why do we need this and setState?

These were added to populate the state with required properties depending on the desired authenticator step during testing. For example, some screens such as the TOTP setup screen expect TotpDetails to be non-null.

Perhaps naming it authenticatorStateOverride would make its purpose more explicit?

Do we need setState as well though? Did setting the initial state via the mocked AuthenticatorState and state machine not work, or is there a need to change the state inside a test?

Do we need setState as well though? is there a need to change the state inside a test?

Yes -- setState is required. The step continueSignInWithMfaSelection requires the actual state to be typed as ContinueSignInWithMfaSelection in order to retrieve it's required values.

Additionally, having one state for all tests was brittle. Any changes to the business logic of state inputs would need to be duplicated in tests and the bloc, ie transforming the Cognito provided TotpSetupDetails into a setupURI string. Instead, we only need to supply the required inputs for a given state. Then we are able to reuse the business logic in the bloc and discretely represent the proper state of each screen of the application.

I'd rather make stateMachineBloc visible for testing and call setState on it than expose a parameter like this

.github/workflows/flutter_vm.yaml

Jordan-Nelson · 2023-09-05T21:17:46Z

packages/authenticator/amplify_authenticator/lib/amplify_authenticator.dart

@@ -437,6 +438,9 @@ class Authenticator extends StatefulWidget {
  /// {@macro amplify_authenticator_dial_code_options}
  final DialCodeOptions dialCodeOptions;

+  @visibleForTesting
+  final AuthenticatorState? mockAuthenticatorState;


Do we need setState as well though? Did setting the initial state via the mocked AuthenticatorState and state machine not work, or is there a need to change the state inside a test?

Equartey · 2023-09-06T16:34:49Z

packages/aft/lib/src/commands/generate/generate_workflows_command.dart

+      ${hasGoldens ? 'runner-os: macos-latest' : ''}
+      ${hasGoldens ? 'has-goldens: true' : ''}


This creates empty lines in other generated workflows when running the generate command.

Couldn't find a simple way to trim the extra lines when not needed...

@dnys1 would it be better to have them always defined?

dnys1 · 2023-09-06T17:14:27Z

.github/workflows/amplify_authenticator.yaml

@@ -9,46 +9,10 @@ on:
    paths:
      - '.github/workflows/amplify_authenticator.yaml'
      - '.github/workflows/flutter_vm.yaml'
-      - 'packages/amplify/amplify_flutter/lib/**/*.dart'


What happened here?

Running the generate workflows command removes several paths in all the workflow files. I only included this one though.

dnys1 · 2023-09-06T17:15:57Z

packages/aft/lib/src/commands/generate/generate_workflows_command.dart

@@ -244,6 +246,8 @@ jobs:
    with:
      package-name: ${package.name}
      working-directory: $repoRelativePath
+      ${hasGoldens ? 'runner-os: macos-latest' : ''}


Remove, it's not a parameter you can pass anyway. But only has-goldens is needed to know which runner to use.

fix: trimmed tests

.github/workflows/flutter_vm.yaml

dnys1 · 2023-09-07T21:04:46Z

packages/authenticator/amplify_authenticator/lib/amplify_authenticator.dart

@@ -437,6 +438,9 @@ class Authenticator extends StatefulWidget {
  /// {@macro amplify_authenticator_dial_code_options}
  final DialCodeOptions dialCodeOptions;

+  @visibleForTesting
+  final AuthenticatorState? mockAuthenticatorState;


I'd rather make stateMachineBloc visible for testing and call setState on it than expose a parameter like this

Jordan-Nelson · 2023-09-08T16:13:29Z

packages/aft/lib/src/commands/generate/generate_workflows_command.dart

@@ -214,6 +214,8 @@ $groupPubPackages
    // TODO(dnys1): Enable E2E runs for Dart packages
    final needsE2ETest = package.flavor == PackageFlavor.flutter &&
        package.integrationTestDirectory != null;
+    final hasGoldens = package.flavor == PackageFlavor.flutter &&
+        package.goldensTestDirectory != null;


An alternative to checking for a directory would be to look for a dev dep of golden_toolkit. It isn't required to run golden tests, but I think we would be likely to use it in any future packages that have golden tests. Looking for a ui directory is fine too. If this is still in place when future UI components are built, we will just have to confirm to that dir.

Understood, this for sure could be different. Since we don't have any other UI packages or other established conventions, we can readdress this down the road.

Jordan-Nelson · 2023-09-08T16:22:01Z

packages/authenticator/amplify_authenticator_test/lib/src/mock_authenticator_app.dart

 import 'package:amplify_flutter/amplify_flutter.dart';
 import 'package:amplify_integration_test/amplify_integration_test.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter_test/flutter_test.dart';

+/// Use for testing the [MockAuthenticatorApp] widget locally.
+void main() {


if you would like to commit this, can you move it to an /example dir and add all the platforms (not just iOS)

…3655)

Equartey requested a review from a team as a code owner August 30, 2023 20:45

Equartey force-pushed the chore/authenticator-totp-goldens branch 7 times, most recently from f10f0ce to 9bc8dae Compare September 1, 2023 17:43

dnys1 reviewed Sep 5, 2023

View reviewed changes

Jordan-Nelson reviewed Sep 5, 2023

View reviewed changes

Equartey commented Sep 6, 2023

View reviewed changes

dnys1 reviewed Sep 6, 2023

View reviewed changes

chore(authenticator_test): Updated Golden tests to run on any screen

cd25066

fix: trimmed tests

Equartey force-pushed the chore/authenticator-totp-goldens branch from 9733d28 to db69ffc Compare September 7, 2023 20:28

dnys1 reviewed Sep 7, 2023

View reviewed changes

dnys1 previously approved these changes Sep 8, 2023

View reviewed changes

Jordan-Nelson reviewed Sep 8, 2023

View reviewed changes

Equartey added 6 commits September 8, 2023 12:43

fix: limit CI to MacOS && save golden failures

0b3ff53

chore: updated golden images

eb8a4ca

fix: updated all flutter_vm workflows to include has-goldens

ad9bbc5

fix: authbloc override refactor

2f7df3c

fix: upload-artifact updated pin

7f2991c

fix: created example app for amplify_authenticator_test

46a25b5

Equartey dismissed dnys1’s stale review via 46a25b5 September 8, 2023 17:45

Equartey force-pushed the chore/authenticator-totp-goldens branch from f8281ce to 46a25b5 Compare September 8, 2023 17:45

Jordan-Nelson approved these changes Sep 8, 2023

View reviewed changes

dnys1 approved these changes Sep 8, 2023

View reviewed changes

Equartey merged commit 9bcac40 into main Sep 8, 2023
117 of 122 checks passed

Equartey deleted the chore/authenticator-totp-goldens branch September 8, 2023 18:37

dnys1 pushed a commit that referenced this pull request Sep 11, 2023

chore(authenticator_test): Updated Golden tests to run on any screen (#…

751ae7a

…3655)

dnys1 pushed a commit that referenced this pull request Sep 11, 2023

chore(authenticator_test): Updated Golden tests to run on any screen (#…

68397be

…3655)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(authenticator_test): Updated Golden tests to run on any screen #3655

chore(authenticator_test): Updated Golden tests to run on any screen #3655

Equartey commented Aug 30, 2023 •

edited

Loading

dnys1 Sep 5, 2023

dnys1 Sep 5, 2023

Equartey Sep 5, 2023

Jordan-Nelson Sep 5, 2023

Equartey Sep 6, 2023 •

edited

Loading

dnys1 Sep 7, 2023 •

edited

Loading

Jordan-Nelson Sep 5, 2023

Equartey Sep 6, 2023

dnys1 Sep 6, 2023

Equartey Sep 6, 2023

dnys1 Sep 6, 2023

dnys1 Sep 7, 2023 •

edited

Loading

Jordan-Nelson Sep 8, 2023

Equartey Sep 8, 2023

Jordan-Nelson Sep 8, 2023

		${hasGoldens ? 'runner-os: macos-latest' : ''}
		${hasGoldens ? 'has-goldens: true' : ''}

chore(authenticator_test): Updated Golden tests to run on any screen #3655

chore(authenticator_test): Updated Golden tests to run on any screen #3655

Conversation

Equartey commented Aug 30, 2023 • 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

Equartey Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

dnys1 Sep 7, 2023 • 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

Choose a reason for hiding this comment

dnys1 Sep 7, 2023 • 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

Equartey commented Aug 30, 2023 •

edited

Loading

Equartey Sep 6, 2023 •

edited

Loading

dnys1 Sep 7, 2023 •

edited

Loading

dnys1 Sep 7, 2023 •

edited

Loading