Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added updateEmail method #69

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bynicodevelop
Copy link

Hello,

I added the updateEmail method to test updating a profile.

Maybe this can interest you.

@atn832
Copy link
Owner

atn832 commented Sep 25, 2022

Thanks for your PR, @bynicodevelop. Would you mind adding a simple unit test to make sure it works and prevent other people from accidentally breaking your code in the future? You can take a look at the tests at https://github.com/atn832/firebase_auth_mocks/blob/master/test/firebase_auth_mocks_test.dart and add yours.

@bynicodevelop
Copy link
Author

Yes, I just did.
I think I follow the existing logic

expect(
() async => await user!.updateEmail('[email protected]'),
returnsNormally,
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add one additional check to verify that the email was updated to the new value?

Also, this test actually changes the value of tUser, which can potentially affect other tests. Would you mind either: 1) initializing your own instance of MockUser in this test
or
2) adding a setUp at the top which reinitializes tUser at every test? Something like:

late MockUser tUser;
setUp(()  {
  tUser = MockUser(
    isAnonymous: false,
    //some other values
  );
});
  1. might be easier for you to do, while 2) is actually the better way, but might require more work. If you do 1), I'll file a ticket to do 2) in a separate PR.

tUser.exception = FirebaseAuthException(code: 'invalid-email');
final user = auth.currentUser;
expect(
() async => await user!.updateEmail('[email protected]'),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try removing async and await here? It should still work. If it does not, feel fee to leave this as is.

See #70.

final auth = MockFirebaseAuth(signedIn: true, mockUser: tUser);
final user = auth.currentUser;
expect(
() async => await user!.updateEmail('[email protected]'),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be able to rewrite this as:

expect(
  () => user!.updateEmail(...),
  completion);

or

expectLater(() => user!.updateEmail(...), returnsNormally);

Can you try? If it does not work, feel free to leave it as is.

See #70.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants