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

tests: email/password confirm user #506

Merged
merged 31 commits into from May 5, 2022
Merged

Conversation

desistefanova
Copy link
Contributor

Create tests for email/password confirm user tokens and auto confirm user.

@cla-bot cla-bot bot added the cla: yes label Apr 27, 2022
@coveralls
Copy link

coveralls commented Apr 27, 2022

Pull Request Test Coverage Report for Build 2276902009

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.677%

Totals Coverage Status
Change from base Build 2276845517: 0.0%
Covered Lines: 400
Relevant Lines: 427

💛 - Coveralls

@desistefanova desistefanova marked this pull request as ready for review April 27, 2022 10:54
@desistefanova desistefanova changed the base branch from master to register_email_tests April 27, 2022 11:03
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Looks good - I have a question and a suggestion

test/credentials_test.dart Show resolved Hide resolved
Comment on lines +113 to +114
"0e6340a446e68fe02a1af1b53c34d5f630b601ebf807d73d10a7fed5c2e996d87d04a683030377ac6058824d8555b24c1417de79019b40f1299aada7ef37fddc",
"6268f7dd73fafea76b730fc9");
Copy link
Member

Choose a reason for hiding this comment

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

How are these tokens generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were sent to my email after registration. I just copied them. They are old tokens, but in a valid format. In this case the validation is throwing different exception.

Copy link
Member

Choose a reason for hiding this comment

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

Hm... I may be wrong, but I don't think we'll be able to use them like that. These are just strings and not JWTs that encode their expiration - I believe they're just stored in the database, so when you try to run this test against a different app, it'll fail with invalid token rather than expired token. Once we fix test runs on CI, I expect this test to fail, but let's see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right, but they are running on the same app in this manual test. It would be good if I had the server code to check the validation, but I couldn't access it.

Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a manual test shouldn't it be skipped by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is not manual. We are passing expired tokens and the exception is thrown.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks

test/test.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@blagoev blagoev left a comment

Choose a reason for hiding this comment

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

some small comments

Comment on lines +113 to +114
"0e6340a446e68fe02a1af1b53c34d5f630b601ebf807d73d10a7fed5c2e996d87d04a683030377ac6058824d8555b24c1417de79019b40f1299aada7ef37fddc",
"6268f7dd73fafea76b730fc9");
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a manual test shouldn't it be skipped by default

}, appName: "emailConfirm");

group("Email/Password - confirm user - manual tests", () {
// The tests in this group are for manual testing, since they require interaction with mail box.
Copy link
Contributor

Choose a reason for hiding this comment

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

could we move the comment to be on the group call. Also could we rename all manual tests to include Manual in the name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Base automatically changed from register_email_tests to master May 5, 2022 16:02
@desistefanova desistefanova merged commit 6ff8e4b into master May 5, 2022
@desistefanova desistefanova deleted the confirm_user_tests branch May 5, 2022 16:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants