Add fxa/sync integration tests #5633

isabelrios · 2019-09-27T09:28:21Z

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
[-] Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
[-] Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

This PR tries to add new tests, sync integration tests, to check the sync process Desktop<->Fenix, first for Bookmarks and in the future for more.

isabelrios · 2019-09-30T11:46:09Z

@rpappalax, @npark-mozilla once the checks are green I would like to ask you to take a first look at the changes in this PR about adding the sync integration tests.
In case of questions please let me know or check the doc with all the implementation details for more info...
Thanks!!

isabelrios · 2019-09-30T11:53:20Z

app/src/androidTest/resources/email.txt

@@ -0,0 +1 @@
+pytest-28f90ad8b4@restmail.net


Note that this is not a permanent account, while running the tests via pipenv run pytesta new account will be created, written in this file and read after that for the test to be used to sync in Fenix app.
Same for the password below.

isabelrios · 2019-09-30T11:54:17Z

automation/taskcluster/androidTest/flank-x86.yml

@@ -33,6 +33,9 @@ gcloud:
    - /sdcard/screenshots
  performance-metrics: true

+  test-targets:


Adding this line and below we avoid running these sync integration tests as part as the regular UI tests.

rpappalax · 2019-09-30T15:14:32Z

app/src/androidTest/java/org/mozilla/fenix/Experiments.kt

+import mozilla.components.service.fretboard.ExperimentDescriptor
+
+const val EXPERIMENTS_JSON_FILENAME = "experiments.json"
+const val EXPERIMENTS_BASE_URL = "https://firefox.settings.services.mozilla.com/v1"


this looks like a production server. Is that intentional for this test?

rpappalax · 2019-09-30T15:19:06Z

app/src/androidTest/java/org/mozilla/fenix/components/FxaServer.kt

+import mozilla.components.service.fxa.ServerConfig
+
+object FxaServer {
+    const val CLIENT_ID = "a2270f727f45f648"


will this CLIENT_ID also be replaced by a dynamic one? (same as: https://github.com/mozilla-mobile/fenix/pull/5633/files/b6603729b13964d8f52aa06342a74601f2bf6cd3#r329536741)? It's OK to hard code these if necessary, but it would be more brittle. Esp. on the stage server

This implementation is similar, using same CLIENT_ID for the non test part, the regular FxaServer.kt. But I can check if this can be changed

rpappalax · 2019-09-30T15:19:46Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/Pipfile

+pytest-fxa = "*"
+pytest-html = "*"
+pytest-metadata = "*"
+requests = "*"


do you not want to pin down these version numbers for stability?

We have had this for the iOS tests for long time and did not face any stability issues...but if you prefer I change that, I can look into that.

Pipfile.lock is checked in, which should pin down dependency versions already.

rpappalax · 2019-09-30T15:29:04Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+    fun checkBookmarkFromDesktopTest() {
+        signInFxSync()
+        tapReturnToPreviousApp()
+        sleep(5000)


i imagine you're going to refine this later, but I recommend creating a constant for this in the helpers. We currently have a few wait times defined in https://github.com/mozilla-mobile/fenix/blob/master/app/src/androidTest/java/org/mozilla/fenix/helpers/TestAssetHelper.kt which was created for Mock server assets / functions. With my next PR, I'll be adding a more generic https://github.com/mozilla-mobile/fenix/blob/master/app/src/androidTest/java/org/mozilla/fenix/helpers/TestHelper.kt which would be for misc functions and constants. We could eventually migrate our constants there.

Yes, these tests, except for the first one are commented out at the moment and will need a little bit of work once the environment is set up and the first test is constantly passing. I just drafted them to have an idea about how the test suite will look like.

rpappalax · 2019-09-30T15:29:27Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+    fun checkHistoryFromDeviceTest() {
+        tapInToolBar()
+        typeInToolBar()
+        sleep(3000)


see above comment

rpappalax · 2019-09-30T15:29:40Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+        val emailInput = mDevice.findObject(UiSelector()
+                .instance(0)
+                .className(EditText::class.java))
+        emailInput.waitForExists(1000)


I will change that to use the constant in helpers. Sorry I forgot about that.

nojunpark · 2019-10-01T15:30:23Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+        tapReturnToPreviousApp()
+        homeScreen {
+        }.openThreeDotMenu {}
+        libraryButton()


this is just a style preference, but I put these methods inside the bracket of openThreeDotMenu to show that these actions are happening within the three dot menu dialog. For me it's more easier to read.

Oh right, when I created this PR those options were not added to the robot menu yet, so I added functions at the end of the test. I will replace them with the actions from robot!

nojunpark · 2019-10-01T15:30:50Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+        val signInButton = mDevice.findObject(UiSelector()
+                .instance(0)
+                .className(Button::class.java))
+        signInButton.waitForExists(10000)


use constants

nojunpark · 2019-10-01T15:31:06Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+
+    fun tapOnContinueButton() {
+        val continueButton = mDevice.findObject(By.res("submit-btn"))
+        continueButton.clickAndWait(Until.newWindow(), 50000)


use constants

nojunpark · 2019-10-01T15:31:15Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+    fun tapOnSygIn() {
+        mDevice.pressEnter()
+        mDevice.wait(Until.findObjects(By.text("Sign in")), 3000)
+        val signInButton = mDevice.findObject(UiSelector()


use constants

nojunpark · 2019-10-01T15:32:26Z

app/src/main/java/org/mozilla/fenix/components/FxaServer.kt

+    fun redirectUrl(context: Context) = if (context.isInExperiment(Experiments.asFeatureWebChannelsDisabled)) {
+        REDIRECT_URL
+    } else {
+        "urn:ietf:wg:oauth:2.0:oob:oauth-redirect-webchannel"


should we put this as a constant?

nojunpark

other than minor style suggestion, LGTM

codecov-io · 2019-10-02T13:34:09Z

Codecov Report

Merging #5633 into master will increase coverage by 0.01%.
The diff coverage is 61.11%.

@@             Coverage Diff              @@
##             master    #5633      +/-   ##
============================================
+ Coverage     14.36%   14.37%   +0.01%     
- Complexity      322      326       +4     
============================================
  Files           257      258       +1     
  Lines         10539    10543       +4     
  Branches       1538     1537       -1     
============================================
+ Hits           1514     1516       +2     
- Misses         8899     8901       +2     
  Partials        126      126

Impacted Files	Coverage Δ	Complexity Δ
...main/java/org/mozilla/fenix/components/Services.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ain/java/org/mozilla/fenix/components/FxaServer.kt	`100% <100%> (ø)`	`4 <4> (?)`
...org/mozilla/fenix/components/BackgroundServices.kt	`46.93% <54.54%> (-1.58%)`	`4 <0> (ø)`
app/src/main/java/org/mozilla/fenix/Config.kt	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf5b4fe...0e3148f. Read the comment docs.

isabelrios · 2019-10-03T14:19:01Z

@csadilek asking for your review since you are familiar with all the changes at least for a first deep look..thanks!

csadilek

Looks good! Just some nits (missing license headers, some KDocs) to address before landing. Thanks for moving this forward!

csadilek · 2019-10-03T15:00:44Z

app/src/main/java/org/mozilla/fenix/components/FxaServer.kt

@@ -0,0 +1,21 @@
+package org.mozilla.fenix.components


We need a license header here :).

csadilek · 2019-10-03T15:03:43Z

app/src/main/java/org/mozilla/fenix/components/FxaServer.kt

+import mozilla.components.service.fxa.ServerConfig
+import org.mozilla.fenix.Experiments
+import org.mozilla.fenix.isInExperiment
+


Let's add some KDocs here e.g.

/** * Utility to configure Firefox Account servers. */

docs/syncIntegrationTests.md

csadilek · 2019-10-03T15:08:21Z

docs/syncIntegrationTests.md

@@ -0,0 +1,26 @@
+### Sync Integration Tests


Thanks for writing these docs!

csadilek · 2019-10-03T15:09:56Z

app/src/androidTest/java/org/mozilla/fenix/components/FxaServer.kt

@@ -0,0 +1,17 @@
+package org.mozilla.fenix.components


We forgot the license header.

csadilek · 2019-10-03T15:10:07Z

app/src/androidTest/java/org/mozilla/fenix/components/FxaServer.kt

+import android.content.Context
+import mozilla.components.service.fxa.ServerConfig
+
+object FxaServer {


Let's add some KDocs, see my comment below.

grigoryk

This is really neat. How do you ensure that FxA signin verification won't be necessary during a test?

grigoryk · 2019-10-03T20:59:22Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/Pipfile

+pytest-fxa = "*"
+pytest-html = "*"
+pytest-metadata = "*"
+requests = "*"


Pipfile.lock is checked in, which should pin down dependency versions already.

grigoryk · 2019-10-03T21:11:09Z

app/src/androidTest/java/org/mozilla/fenix/Experiments.kt

+
+fun Context.isInExperiment(descriptor: ExperimentDescriptor): Boolean =
+    if (descriptor.name == "asFeatureWebChannelsDisabled")
+        true


The only reason webchannel stuff didn't work is because we didn't whitelist stage FxA server in the webchannel extension manifest file. We certainly do want these tests to exercise webchannel flow, since that's the flow we ship by default. This can be an easy follow-up PR though.

grigoryk · 2019-10-03T21:20:42Z

app/src/androidTest/java/org/mozilla/fenix/syncintegration/SyncIntegrationTest.kt

+        passwordInput.setText(passwordValue)
+    }
+
+    fun tapOnSygIn() {


Right! 🙈

grigoryk · 2019-10-03T21:24:29Z

app/src/test/java/org/mozilla/fenix/components/BackgroundServicesTest.kt

@@ -50,10 +50,10 @@ class BackgroundServicesTest {
        val context = mockk<Context>(relaxed = true)

        every { context.isInExperiment(eq(Experiments.asFeatureWebChannelsDisabled)) } returns false
-        assertEquals("urn:ietf:wg:oauth:2.0:oob:oauth-redirect-webchannel", BackgroundServices.redirectUrl(context))
+        // assertEquals("urn:ietf:wg:oauth:2.0:oob:oauth-redirect-webchannel", BackgroundServices.redirectUrl(context))


This seems wrong. Why did you need to disable these tests?

Ah this was likely because redirectUrl is no longer a function on BackgroundServices. Didn't notice this was commented out. So, we can change that to FxaServer.redirectUrl(context) now and it should work again.

Oh, sorry about that I commented it so that building on TC did not complain and forgot to fix. Fixing...

isabelrios · 2019-10-04T08:19:25Z

This is really neat. How do you ensure that FxA signin verification won't be necessary during a test?

The verification happens after the account is created with fxacli tool in plugin.py (part of the code):

....
fxa_account = FxAccount(email=account.email, password=password)
    try:
        session = fxa_client.create_account(fxa_account.email,
                                            fxa_account.password)
        logger.info('Created: {}'.format(fxa_account))
        account.fetch()
        message = account.wait_for_email(
            lambda m: 'x-verify-code' in m['headers'] and
            session.uid == m['headers']['x-uid']
        )
        assert (message is not None), 'Verification email not found.'
        session.verify_email_code(message['headers']['x-verify-code'])
        logger.info('Verified: {}'.format(fxa_account))
        yield fxa_account
....

In the logs you can see something as:

root:plugin.py:93 Created: FxAccount(email='pytest-9e83976acb@restmail.net', password='cwyOfVGK')
root:plugin.py:101 Verified: FxAccount(email='pytest-9e83976acb@restmail.net', password='cwyOfVGK')

When the test finishes, the account is removed.

fixing Jenkins path to tests and clean tests commenting future tests and adding doc with info about the tests

improve writing email and password in file

sblatz · 2019-10-08T21:15:21Z

Is this ready to land? This PR has been up and approved for a while. Want to make sure I don't merge it before all nits have been addressed though.

isabelrios · 2019-10-09T11:37:30Z

@sblatz yes, it should be ready, but if you want me to double check with one (or more) reviewers, let me know...

isabelrios · 2019-10-09T11:38:22Z

This is really neat. How do you ensure that FxA signin verification won't be necessary during a test?

The verification happens after the account is created with fxacli tool in plugin.py (part of the code):

....
fxa_account = FxAccount(email=account.email, password=password)
    try:
        session = fxa_client.create_account(fxa_account.email,
                                            fxa_account.password)
        logger.info('Created: {}'.format(fxa_account))
        account.fetch()
        message = account.wait_for_email(
            lambda m: 'x-verify-code' in m['headers'] and
            session.uid == m['headers']['x-uid']
        )
        assert (message is not None), 'Verification email not found.'
        session.verify_email_code(message['headers']['x-verify-code'])
        logger.info('Verified: {}'.format(fxa_account))
        yield fxa_account
....

In the logs you can see something as:

root:plugin.py:93 Created: FxAccount(email='pytest-9e83976acb@restmail.net', password='cwyOfVGK')
root:plugin.py:101 Verified: FxAccount(email='pytest-9e83976acb@restmail.net', password='cwyOfVGK')

When the test finishes, the account is removed.

@grigoryk I replied to your question but did not mention you so not sure if you read it and that answers your question...Thanks!

isabelrios · 2019-10-11T10:01:09Z

Hi @sblatz , what's that bors expected? Should I do something before merging?

sblatz · 2019-10-11T14:43:21Z

bors r=csadilek

5633: Add fxa/sync integration tests r=csadilek a=isabelrios Pull Request checklist  - [x] **Quality**: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended) - [x] **Tests**: This PR includes thorough tests or an explanation of why it does not - [-] **Screenshots**: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not - [-] **Accessibility**: The code in this PR follows [accessibility best practices](https://github.com/mozilla-mobile/shared-docs/blob/master/android/accessibility_guide.md) or does not include any user facing features This PR tries to add new tests, sync integration tests, to check the sync process Desktop<->Fenix, first for Bookmarks and in the future for more. Co-authored-by: Isabel Rios <isabelrios@mackbookirios.home> Co-authored-by: isabelrios <isabelrios@gmail.com>

bors · 2019-10-11T15:24:44Z

Build succeeded

build-debug
lint-compare-locales
lint-detekt
lint-ktlint
lint-lint
test-debug
test-ui

isabelrios added the pr:do-not-land label Sep 27, 2019

isabelrios commented Sep 30, 2019

View reviewed changes

rpappalax reviewed Sep 30, 2019

View reviewed changes

nojunpark reviewed Oct 1, 2019

View reviewed changes

nojunpark approved these changes Oct 1, 2019

View reviewed changes

isabelrios force-pushed the sync-integration-tests branch 2 times, most recently from 55a641d to edb879c Compare October 2, 2019 19:55

isabelrios marked this pull request as ready for review October 3, 2019 10:37

isabelrios force-pushed the sync-integration-tests branch from edb879c to 26350d4 Compare October 3, 2019 10:45

isabelrios requested a review from csadilek October 3, 2019 14:18

csadilek approved these changes Oct 3, 2019

View reviewed changes

grigoryk reviewed Oct 3, 2019

View reviewed changes

isabelrios force-pushed the sync-integration-tests branch from 5159d43 to 7077d34 Compare October 7, 2019 08:26

isabelrios changed the title ~~(WIP) Add fxa/sync integration tests~~ Add fxa/sync integration tests Oct 7, 2019

isabelrios removed the pr:do-not-land label Oct 7, 2019

Add fxa/sync integration tests

44f4ced

fixing Jenkins path to tests and clean tests commenting future tests and adding doc with info about the tests

isabelrios added 3 commits October 8, 2019 09:56

Fix TC error and run only ui tests

cdefb0d

fixing reviewers comments

2fc6eaf

improve writing email and password in file

fixing more reviewers comments

0e3148f

isabelrios force-pushed the sync-integration-tests branch from 7077d34 to 0e3148f Compare October 8, 2019 07:56

sblatz added the pr:do-not-land label Oct 8, 2019

sblatz removed the pr:do-not-land label Oct 11, 2019

bors bot merged commit 0e3148f into mozilla-mobile:master Oct 11, 2019

		@@ -0,0 +1 @@
		pytest-28f90ad8b4@restmail.net

Add fxa/sync integration tests #5633

Add fxa/sync integration tests #5633

Conversation

isabelrios commented Sep 27, 2019

isabelrios commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojunpark left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 2, 2019 • edited Loading

Codecov Report

isabelrios commented Oct 3, 2019

csadilek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grigoryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelrios commented Oct 4, 2019 • edited Loading

sblatz commented Oct 8, 2019

isabelrios commented Oct 9, 2019

isabelrios commented Oct 9, 2019

isabelrios commented Oct 11, 2019

sblatz commented Oct 11, 2019

bors bot commented Oct 11, 2019

Build succeeded

codecov-io commented Oct 2, 2019 •

edited

Loading

isabelrios commented Oct 4, 2019 •

edited

Loading