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.

Sign up for GitHub

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

Jump to bottom

Fixed tests so that they expect button to the disabled #1098 #1099

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fixed tests so that they expect button to the disabled #1098 #1099

wants to merge 6 commits into from

Conversation

epicadk
Copy link
Member

@epicadk epicadk commented Feb 24, 2021 •
edited
Loading

Description

Currently, the tests expect the buttons to be enabled even if the user has not entered all required information. Updated tests so that they expect buttons to be disabled. I have not added any more tests rather I have just corrected the present tests to expect the response that was discussed in the issue. All tests except one sign up tests pass. The signup test does not pass as there is an open pr that solves #756 and only then the test in question will pass.
Fixes #1098

Type of Change:

  • Code
  • Quality Assurance

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Ran the test on my pc.

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

epicadk
epicadk commented Feb 24, 2021
View reviewed changes
app/src/androidTest/java/org/systers/mentorship/SignUpActivityTest.kt
findEditTextInTextInputLayout(R.id.tiEmail).perform(typeText("[email protected]"), closeSoftKeyboard())
findEditTextInTextInputLayout(R.id.tiPassword).perform(typeText("Divyansh@2001"), closeSoftKeyboard())
findEditTextInTextInputLayout(R.id.tiConfirmPassword).perform(typeText("Divyansh@2001"), closeSoftKeyboard())
findEditTextInTextInputLayout(R.id.tiUsername).perform(typeText("isabel"), closeSoftKeyboard())
Copy link
Member Author

Choose a reason for hiding this comment

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

@isabelcosta sorry I couldn't really think of any other username so I used your, if we do end up creating a user solely for testing we can update it here.

Copy link
Member

Choose a reason for hiding this comment

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

username is fine ;)

@epicadk epicadk mentioned this pull request Mar 4, 2021
Open
kartikeysaran
kartikeysaran previously approved these changes Mar 7, 2021
View reviewed changes
@justdvnsh
Copy link
Contributor

justdvnsh commented Mar 7, 2021 •
edited
Loading

@epicadk The tests are failing for me. Also, there is one caveat, I forgot to think about. The whenEmailIsAlreadyRegistered will fail, in case of a server timeout. We need to take that into account. Also, if this is the case, then maybe, whenUsernameIsAlreadyRegistered will also fail, in case of a timeout. Should I open a new issue for it ?

anita-borg-issue_epicadk
anita-borg-issue_epicadk-2

@epicadk
Copy link
Member Author

epicadk commented Mar 7, 2021

@epicadk The tests are failing for me. Also, there is one caveat, I forgot to think about. The whenEmailIsAlreadyRegistered will fail, in case of a server timeout. We need to take that into account. Also, if this is the case, then maybe, whenUsernameIsAlreadyRegistered will also fail, in case of a timeout. Should I open a new issue for it ?

anita-borg-issue_epicadk
anita-borg-issue_epicadk-2

Yes the test will fail because it depends on the other pr I had linked.

Sure you can create another issue. This issue only involved correcting the existing tests.

@justdvnsh
Copy link
Contributor

@epicadk Yup , I thought so. Cool, I'll be reviewing the other PR and also creating new issues as well.

isabelcosta
isabelcosta previously approved these changes Mar 27, 2021
View reviewed changes
Copy link
Member

@isabelcosta isabelcosta 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 need more eyes on this, as this is not my forte anymore 🙈
Thank you for working on this @epicadk !

@vj-codes vj-codes added the Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. label Apr 1, 2021
vj-codes
vj-codes previously approved these changes Apr 1, 2021
View reviewed changes
Copy link
Member

@vj-codes vj-codes left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@epicadk epicadk dismissed stale reviews from vj-codes and isabelcosta via 07eaae1 April 4, 2021 06:18
@epicadk
Copy link
Member Author

epicadk commented Apr 10, 2021

@vj-codes can you please review this?

@vj-codes
Copy link
Member

@epicadk yes also I tried testing it I got this error -

What went wrong:
Execution failed for task ':app:connectedDebugAndroidTest'.
> com.android.builder.testing.api.DeviceException: No connected devices!
BUILD FAILED 
53 actionable tasks: 35 executed, 18 up-to-date

I do have my pixel emulator connected or does it need a physical device?

@epicadk
Copy link
Member Author

epicadk commented Apr 11, 2021

@epicadk yes also I tried testing it I got this error -

What went wrong:
Execution failed for task ':app:connectedDebugAndroidTest'.
> com.android.builder.testing.api.DeviceException: No connected devices!
BUILD FAILED 
53 actionable tasks: 35 executed, 18 up-to-date

I do have my pixel emulator connected or does it need a physical device?

Yeah it needs a Physical device

vj-codes
vj-codes requested changes Jun 15, 2021
View reviewed changes
Copy link
Member

@vj-codes vj-codes left a comment

Choose a reason for hiding this comment

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

@epicadk please resolve the merge conflicts when you're free and I can probably test this again

@vj-codes vj-codes added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vj-codes vj-codes vj-codes requested changes

@kartikeysaran kartikeysaran kartikeysaran left review comments

@isabelcosta isabelcosta isabelcosta left review comments

@gaurivn gaurivn Awaiting requested review from gaurivn

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Status: Changes Requested Changes are required to be done by the PR author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug : fix login signup and confirm tests to expect buttons disabled.
5 participants
@epicadk @justdvnsh @vj-codes @isabelcosta @kartikeysaran