First integration test: Sign in with email address and password #175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ded from addStream`
… integration tests
nilsreichardt
commented
May 11, 2022
|
Visit the preview URL for this PR (updated for commit 0ac58f3): https://sharezone-test--pr175-integration-test-sig-lvbq1x2w.web.app (expires Mon, 23 May 2022 15:52:23 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
|
I'm getting build errors :( Build errors |
Jonas-Sander
requested changes
May 12, 2022
Comment on lines
+22
to
+36
| _Note: The integration tests are not working for the web at the moment because we need to migrate our app to null safety. Otherwise, the build will fail because of this message:_ | ||
| ``` | ||
| org-dartlang-app:/app_test.dart:27:11: Error: Non-nullable variable 'dependencies' must be assigned before it can be used. | ||
| await dependencies.blocDependencies.auth.signOut(); | ||
| ^^^^^^^^^^^^ | ||
| org-dartlang-app:/app_test.dart:34:30: Error: Non-nullable variable 'dependencies' must be assigned before it can be used. | ||
| beitrittsversuche: dependencies.beitrittsversuche, | ||
| ^^^^^^^^^^^^ | ||
| org-dartlang-app:/app_test.dart:35:29: Error: Non-nullable variable 'dependencies' must be assigned before it can be used. | ||
| blocDependencies: dependencies.blocDependencies, | ||
| ^^^^^^^^^^^^ | ||
| org-dartlang-app:/app_test.dart:36:28: Error: Non-nullable variable 'dependencies' must be assigned before it can be used. | ||
| dynamicLinkBloc: dependencies.dynamicLinkBloc, | ||
| ^^^^^^^^^^^^ | ||
| ``` |
There was a problem hiding this comment.
It seems like this might happen because on web it runs with null-safety?
Couldn't we either:
- Use
latefor group variables
late AppDependencies dependencies;
late _UserCredentials user1;-
Pass
--no-sound-null-safetyexplicitly -
Make the
app_testfile use null-safety by using// @dart=2.14
(I haven't tried any of it - just general ideas.)
There was a problem hiding this comment.
I also tried this, but you will get a different error other packages require null safety.
app/integration_test/app_test.dart
Outdated
| _UserCredentials user1; | ||
|
|
||
| setUpAll(() async { | ||
| dependencies = await initializeFlutterApp(isIntegrationTest: true); |
There was a problem hiding this comment.
No problem for now but does it fundamentally even make sense that the app returns dependencies? Wouldn't we pass dependencies to the app to run?
I mean it might make sense to pass dependencies and still have a way to access the objects created with those dependencies, I guess? Idk.
Like I said it's okay for now, but it kinda confuses me.
There was a problem hiding this comment.
Yeah, we can definitely rework this, but it requires some refactoring in the main.dart because we do so many dirty things in the runFlutterApp method 😅 Therefore, my plan is tough in the app the thing which are blocking the integration tests, write a lot of integration tests and then start refactoring.
|
@Jonas-Sander Can you re-review? |
Jonas-Sander
approved these changes
May 13, 2022
nilsreichardt
commented
May 14, 2022
app/lib/main/run_app.dart
Outdated
Comment on lines
159
to
171
| // When the user signs out, we need to dispose the listeners and stream | ||
| // subscriptions inside the gateways. Otherwise, we we would cause a | ||
| // memory leak and receiving a permission denied error from Firestore | ||
| // because we would try to access the user data after the user signed out. | ||
| // This would result an instant fail of the integration tests. | ||
|
|
||
| if (userGateway != null) { | ||
| await userGateway.dispose(); | ||
| } | ||
|
|
||
| if (sharezoneGateway != null) { | ||
| await sharezoneGateway.dispose(); | ||
| } |
There was a problem hiding this comment.
@Jonas-Sander I have managed to modify SharezoneGateway, UserGateway, ConnectionsGateway, DataDocumentPackage and DataCombinedPackage a bit so that we are able to dispose them correctly (not for the whole app but for the listenToAuthStateChanged listener). Therefore, we can remove is isIntegrationTest flag 🙂
There was a problem hiding this comment.
But I need to check manually that this doesn't break anything in the app.
There was a problem hiding this comment.
Didn't find any issues for now.
There was a problem hiding this comment.
Nit: Might it be more readable to do it as a pre-check via if (user.typeOfUser == null) instead of running it inside the else clause?
Also couldn't one do userGateway?.dispose()?
Jonas-Sander
approved these changes
May 16, 2022
app/lib/main/run_app.dart
Outdated
Comment on lines
159
to
171
| // When the user signs out, we need to dispose the listeners and stream | ||
| // subscriptions inside the gateways. Otherwise, we we would cause a | ||
| // memory leak and receiving a permission denied error from Firestore | ||
| // because we would try to access the user data after the user signed out. | ||
| // This would result an instant fail of the integration tests. | ||
|
|
||
| if (userGateway != null) { | ||
| await userGateway.dispose(); | ||
| } | ||
|
|
||
| if (sharezoneGateway != null) { | ||
| await sharezoneGateway.dispose(); | ||
| } |
There was a problem hiding this comment.
Nit: Might it be more readable to do it as a pre-check via if (user.typeOfUser == null) instead of running it inside the else clause?
Also couldn't one do userGateway?.dispose()?
I don't why but I can't answer to your comment, so I'm adding a new comment. I would keep in the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces the first integration test. It's a simple one: Sign in with the email address and the password. The main focus of the PR was to set up the integration and fix the errors. Therefore, we can add more integration tests in different PRs.
Demo
Screen.Recording.2022-05-11.at.14.02.54.mov
How to run the integration tests
Native setup for integration tests
I skipped the steps for setting up the iOS and Android device tests, because I think we only need these setups when run our tests with Firebase Test Lab or similar services. Firebase Test Lab needs these steps to properly run the tests. But at the moment, we just run our integration tests with
flutter testin emulators/simulators.Related Tickets
Closes #180