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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: isabel_rios, Assigned: isabel_rios)

Tracking

unspecified
Firefox 65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

7 months ago
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!
Flags: needinfo?(markh)
(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.
Flags: needinfo?(markh)
Assignee

Comment 2

7 months ago
(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.
Assignee

Comment 3

7 months ago
SyncIntegration firefox-ios tests are failing if the server is wiped in first phase
Assignee

Comment 4

7 months ago
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!
Flags: needinfo?(markh)
(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 :)
Flags: needinfo?(markh)
Assignee

Comment 6

7 months ago
> 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!
Assignee

Updated

7 months ago
Assignee: nobody → irios.mozilla

Comment 7

7 months ago
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/868c763e5d10
TPS do not wipe server at first phase r=markh

Comment 8

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/868c763e5d10
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.