Closed Bug 838596 Opened 8 years ago Closed 8 years ago

Robocop: Add test for 'Master Password' feature

Categories

(Firefox for Android :: General, defect)

21 Branch
ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: paul.feher, Assigned: paul.feher)

References

Details

Attachments

(1 file, 4 obsolete files)

This bug tracks the Robocop tests part of 'Master Password' feature on Firefox for Android.
This patch tests the Master Password feature first by enabling the password, then testing it on a login page and finally disabling the password.
This patch integrates the following test cases:
Clear private data will not clear the saved Master Password
Characters typed in Master Password field(s) are converted to dots
Disable Master Password
Confirm a wrong Master Password while saving new credentials to Password Manager
Confirm the correct Master Password while saving new credentials to Password Manager
Set Master Password
Disable Master Password using a wrong password
Master Password popup is triggered when a password is remembered
https://moztrap.mozilla.org/manage/cases/?filter-suite=79&pagenumber=2
Attached patch MasterPassword V1 (obsolete) — Splinter Review
Attachment #710693 - Flags: review?(jmaher)
Comment on attachment 710693 [details] [diff] [review]
MasterPassword V1

Review of attachment 710693 [details] [diff] [review]:
-----------------------------------------------------------------

overall the indentation level needs a lot of work.  This fails on the tegras both for me locally and on try server:

FIRE PROC: '"MOZ_CRASHREPORTER=1,XPCOM_DEBUG_BREAK=stack,MOZ_HIDE_RESULTS_TABLE=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1,MOZ_PROCESS_LOG=/tmp/tmpilWlJRpidlog,XPCOM_MEM_BLOAT_LOG=/tmp/tmp6Nzof5/runtests_leaks.log" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec_jmaher.tests.testMasterPassword org.mozilla.roboexample.test/org.mozilla.fennec_jmaher.FennecInstrumentationTestRunner'
Robocop derived process name: org.mozilla.fennec_jmaher
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testMasterPassword
2 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until both fields are filled
3 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until the Confirm password field is filled
4 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until until both fields contain the same password
5 INFO TEST-PASS | testMasterPassword | Verify if the OK button is inactive - The OK button is inactive until the Password field is filled
6 INFO TEST-PASS | testMasterPassword | Checking if no password was set if the action was canceled - No password was set
7 INFO TEST-PASS | testMasterPassword | waiting to convert the letters in dots - The letters are converted in dots
8 INFO TEST-PASS | testMasterPassword | Checking if the password is enabled - The password is enabled
9 INFO TEST-PASS | testMasterPassword | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_login.html should equal http://mochi.test:8888/tests/robocop/robocop_login.html
10 INFO TEST-PASS | testMasterPassword | Doorhanger notification is hidden - false should equal false
11 INFO TEST-PASS | testMasterPassword | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_login.html should equal http://mochi.test:8888/tests/robocop/robocop_login.html
Exception caught during test!
junit.framework.AssertionFailedError: The text: OK is not found!
	at junit.framework.Assert.fail(Assert.java:47)
	at junit.framework.Assert.assertTrue(Assert.java:20)
	at com.jayway.android.robotium.solo.Clicker.clickOnText(Clicker.java:291)
	at com.jayway.android.robotium.solo.Solo.clickOnText(Solo.java:823)
	at org.mozilla.fennec_jmaher.tests.testMasterPassword.verifyLoginPage(testMasterPassword.java:171)
	at org.mozilla.fennec_jmaher.tests.testMasterPassword.testMasterPassword(testMasterPassword.java:24)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:135)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
12 INFO TEST-UNEXPECTED-FAIL | testMasterPassword | Exception caught - junit.framework.AssertionFailedError: The text: OK is not found!
13 INFO TEST-END | testMasterPassword | finished in 117508ms
14 INFO TEST-START | Shutdown
15 INFO Passed: 10
16 INFO Failed: 1
17 INFO Todo: 0
18 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:02:01.520218

::: mobile/android/base/tests/testMasterPassword.java.in
@@ +5,5 @@
> +
> +/* This patch tests the Master Password feature first by enabling the password,
> +then testing it on a login page and finally disabling the password */
> +public class testMasterPassword extends PixelTest {
> +	Device dev;

nit: 4 spaces only.

@@ +22,5 @@
> +
> +		enableMasterPassword(password, badPassword);
> +		verifyLoginPage(password, badPassword);
> +		disableMasterPassword(password, badPassword);
> +    }

nit: 4 space indent please.
Attachment #710693 - Flags: review?(jmaher) → review-
Attached patch MasterPassword V2 (obsolete) — Splinter Review
I've made the necessary changes.
Attachment #710693 - Attachment is obsolete: true
Attachment #712891 - Flags: review?(jmaher)
Attached patch MasterPassword V2.1 (obsolete) — Splinter Review
clearing some spaces
Attachment #712891 - Attachment is obsolete: true
Attachment #712891 - Flags: review?(jmaher)
Attachment #712896 - Flags: review?(jmaher)
Comment on attachment 712896 [details] [diff] [review]
MasterPassword V2.1

Review of attachment 712896 [details] [diff] [review]:
-----------------------------------------------------------------

this is a comprehensive test case and it covers this feature very well.  We should be doing 4 space indentation, not 8.  

r- for it failing on the tegras.  I am not sure why as I tried a few things to get this to click.

::: mobile/android/base/tests/testMasterPassword.java.in
@@ +29,5 @@
> +
> +		// Look for the 'Settings' menu if this device/OS uses it
> +		selectMenuItem("Settings");
> +		mSolo.waitForText("^Use master password$");
> +		mSolo.clickOnText("^Use master password$");

this never happens on the tegras (android 2.3 tablet mode).  I have verified via a screenshot that we are opening up the settings menu and this text is visible.

@@ +165,5 @@
> +
> +		mSolo.waitForText("Enter Search or Address");// Wait for the page to load
> +		for (String item:option) {
> +	 		if (!item.equals("Not Now")) {
> +				if (!item.equals("Never")) {

this logic is confusing.  I would do 3 methods "Remember", "Never", "NotNow" and put the logic in there.  You could get rid of this crazy if block and for stuff and just do:
verifyPasswordRemember(LOGIN_URL, password, badPassword);
verifyPasswordNever(LOGIN_URL, password, badPassword);
verifyPasswordNotNow(LOGIN_URL, password, badPassword);
Attachment #712896 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #5)
> Comment on attachment 712896 [details] [diff] [review]
> MasterPassword V2.1
> 
> Review of attachment 712896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this is a comprehensive test case and it covers this feature very well.  We
> should be doing 4 space indentation, not 8.  
> 
> r- for it failing on the tegras.  I am not sure why as I tried a few things
> to get this to click.
> 
> ::: mobile/android/base/tests/testMasterPassword.java.in
> @@ +29,5 @@
> > +
> > +		// Look for the 'Settings' menu if this device/OS uses it
> > +		selectMenuItem("Settings");
> > +		mSolo.waitForText("^Use master password$");
> > +		mSolo.clickOnText("^Use master password$");
> 
> this never happens on the tegras (android 2.3 tablet mode).  I have verified
> via a screenshot that we are opening up the settings menu and this text is
> visible.

Some ideas:
 - maybe the ^ or $ is a problem? (maybe there is a leading or trailing space in the menu?)
 - maybe we need to click on the check box rather than the text? I know clicking on the text works on some phones, but maybe that's not universal.
I removed the ^ and the $ from the waitForText as well as another run on the clickOnText.  It didn't help.  Maybe the checkbox is the solution,
Status: NEW → ASSIGNED
Comment on attachment 712896 [details] [diff] [review]
MasterPassword V2.1

I retested on try server with the latest bits of everthing, lots of green with many retriggers.
Attachment #712896 - Flags: review- → review+
Comment on attachment 712896 [details] [diff] [review]
MasterPassword V2.1

oh, I was wrong.  This patch doesn't have the robocop.ini bits added to it, so when I pushed again with the test activated I see the failure I indicated originally.

Here is a link to the try server push:
https://tbpl.mozilla.org/?tree=Try&rev=fce593720fc3&noignore=1
Attachment #712896 - Flags: review+ → review-
The patch on bug 837318 -- landing today -- might help here.
I tried another try job this morning and it appears to still be problematic:
https://tbpl.mozilla.org/?tree=Try&rev=1a1577667848&noignore=1
Blocks: 744859
Attached patch MasterPassword V3 (obsolete) — Splinter Review
I think this patch solves the problem, looks ok on try-server
https://tbpl.mozilla.org/?tree=Try&rev=b0ca8e8c2b43
Attachment #712896 - Attachment is obsolete: true
Attachment #749863 - Flags: review?(jmaher)
Comment on attachment 749863 [details] [diff] [review]
MasterPassword V3

Review of attachment 749863 [details] [diff] [review]:
-----------------------------------------------------------------

this patch looks good with one nit.  The try server run has to many oranges and blues to land this.  I want to make sure we get this small backlog of tests landed in the next week or two.

::: mobile/android/base/tests/testMasterPassword.java.in
@@ +70,5 @@
> +        mActions.sendKeys(password);
> +        mSolo.clickOnEditText(1);
> +        mActions.sendKeys(password);
> +
> +        // Verify that the imput caracters ar convered to dots automaticaly

nit: // Verify that the input characters are converted to dots automatically
Attachment #749863 - Flags: review?(jmaher) → review+
I made the necessary adjustments and I removed the by-pass section in the code cause by Bug871464 since this was fixed. Now the try server run looks good, much greener.
https://tbpl.mozilla.org/?tree=Try&rev=d087a57c1027
Attachment #749863 - Attachment is obsolete: true
Attachment #756475 - Flags: review?(jmaher)
Comment on attachment 756475 [details] [diff] [review]
MasterPassword V4

Review of attachment 756475 [details] [diff] [review]:
-----------------------------------------------------------------

try server looks a lot better, this patch in general looks good.
Attachment #756475 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/mozilla-central/rev/e9193d5774f4
Assignee: nobody → paul.feher
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Depends on: 878587
Depends on: 878589
Depends on: 879089
Depends on: 879767
Depends on: 880005
Depends on: 887119
Depends on: 891084
You need to log in before you can comment on or make changes to this bug.