1502109 - [TPS] Wiping the server for the first phase make firefox-ios sync integration tests fail

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Isabel Rios[:isabel_rios]]

Firefox-iOS Sync Integration tests start by saving data on mobile, then loging in and then checking it on desktop with same FxAccount.

https://github.com/mozilla-mobile/firefox-ios/tree/master/SyncIntegrationTests

There are several scenarios:
-Bookmark
-Tabs
-History
-Logins

Now looks like the tps have changed. It wipes the server when it is launched
https://searchfox.org/mozilla-central/source/services/sync/tps/extensions/tps/resource/tps.jsm#1049-1051

Phase: 
function Test__Phase(phasename, fnlist) {
    if (Object.keys(this._phaselist).length === 0) {
      // This is the first phase we should wipe the server - this has the
      // side-effect of forcing a login, which we also need.
      fnlist.unshift([this.WipeServer]);
    }
    this._phaselist[phasename] = fnlist;
  }

So, the data generated by firefox-iOS is removed and so the tests fail when trying to find it on desktop. There is nothing, no previous bookmark saved is synchronized.

Would it be possible to make this optional or to add a preference to determine if the server should be wiped?

Thanks!

Comment 1

[Mark Hammond [:markh] [:mhammond]]

(In reply to Isabel Rios[:isabel_rios] from comment #0)
>       // This is the first phase we should wipe the server - this has the
>       // side-effect of forcing a login, which we also need.
>       fnlist.unshift([this.WipeServer]);

Changing this to `fnlist.unshift([this.Login]);` will probably work - I made that change as I noticed that a cleanup phase failing to complete could cause the next run to fail but didn't realize it would break your work - sorry about that.

Comment 2

[Isabel Rios[:isabel_rios]]

(In reply to Mark Hammond [:markh] from comment #1)

> Changing this to `fnlist.unshift([this.Login]);` will probably work - I made
> that change as I noticed that a cleanup phase failing to complete could
> cause the next run to fail but didn't realize it would break your work -
> sorry about that.

Thanks Mark! no worries, too many tests and scenarios to take into account.
Changing that line as you suggested fixes the issue for us. I will try to create a PR with that change.

Comment 3

[Isabel Rios[:isabel_rios]]

Attachment: Bug 1502109 - TPS do not wipe server at first phase

SyncIntegration firefox-ios tests are failing if the server is wiped in first phase

Comment 4

[Isabel Rios[:isabel_rios]]

Sorry Mark, Dave, I was expecting some feedback here and just realized you commented on the PR instead (very new to that process...)

> Are we sure this will fix your issue? 

Yes, our tests are running locally with this change


> Are you able to run tps tests against custom builds?

Not sure how to do that...I was following the steps here:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/TPS_Tests

Using a testing restmail account (a production one) and the firefox client I have installed

But getting an error so far "RunnerNotStartedError: Failed to start the process: [Errno 13] Permission denied" when trying to run the tests.

Is that what I need to run or you mean other tests? Any other clue to run the test that I may be missing?

Thanks!

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Isabel Rios[:isabel_rios] from comment #4)
> Sorry Mark, Dave, I was expecting some feedback here and just realized you
> commented on the PR instead (very new to that process...)

No probs at all! And sorry I wasn't clear - I asked the same question twice :(

> > Are we sure this will fix your issue? 
> 
> Yes, our tests are running locally with this change

Great - I just wanted to make sure you were able to test these changes other than by having them land.

> > Are you able to run tps tests against custom builds?
> 
> Not sure how to do that...I was following the steps here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/TPS_Tests

Here I was asking the same question - but I don't understand what "running locally" means for you other than this :)
 
> Is that what I need to run or you mean other tests? Any other clue to run
> the test that I may be missing?

No - that doesn't ring a bell at all - although so long as there's some process to ensure your patches get results before we land them, I'm good - just make the comment change in phabricator and we can land it :)

Comment 6

[Isabel Rios[:isabel_rios]]

> Here I was asking the same question - but I don't understand what "running locally" means for you other than this :)

Oh sorry I did not get what you mean :)

I did the change and run the test on my machine with a tps build including the change. Kind of local environment to check that the tests were working, that's whay I meant by locally...

> No - that doesn't ring a bell at all - although so long as there's some process to ensure your patches get results before we land them

Oh okay, I finally got to launch the tps tests and see the results on my laptop, although it is not useful for checking the patch, at least I learnt how to run them. 

> I'm good - just make the comment change in phabricator and we can land it :)

I just changed the comment, hope it looks better now.

Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/868c763e5d10
TPS do not wipe server at first phase r=markh

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/868c763e5d10