Closed Bug 995599 Opened 6 years ago Closed 6 years ago

services xpcshell tests go beyond localhost

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- disabled
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: mcmanus, Assigned: markh)

References

Details

(Whiteboard: p=5 s=33.1 [qa-])

Attachments

(1 file, 1 obsolete file)

The test infrastrucutre is meant to be self contained - so xpcshell, mochitests, and crashtests need to be self hosted.

services/fxaccounts/tests/xpcshell/test_accounts.js connects to api.accounts.firefox.com and ocsp.digitcert.com. (the latter is probably to check revocation of a cert from the former)

services/sync/tests/unit/test_fxa_startOver.js connects to auth.services.mozilla.com, ocsp.digitcert.com (again, related to a cert, and phx-sync-13-3-4.services.mozilla.com

these should be fixed to be localhost only.
Mark, your name appears prominently as a reviewer and an author of some of these tests.  Any chance we can just set prefs to make the non-local accesses for these go away as easily as bug 999518?
Flags: needinfo?(mhammond)
This patch *seems* to solve the problem, but I'm mainly going on the logs rather than looking at a network trace.  Nathan, is it simple for you to test this patch does indeed solve the problem?  I'll then find a reviewer.
Attachment #8422247 - Flags: feedback?(nfroyd)
Flags: needinfo?(mhammond) → needinfo?(nfroyd)
(In reply to Mark Hammond [:markh] from comment #2)
> This patch *seems* to solve the problem, but I'm mainly going on the logs
> rather than looking at a network trace.  Nathan, is it simple for you to
> test this patch does indeed solve the problem?  I'll then find a reviewer.

No network problems on Linux, one comment on the patch here, though...
Flags: needinfo?(nfroyd)
Comment on attachment 8422247 [details] [diff] [review]
0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

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

::: services/sync/tests/unit/test_fxa_startOver.js
@@ +27,5 @@
>    // we expect the "legacy" provider (but can't instanceof that, as BrowserIDManager
>    // extends it)
>    do_check_false(Service.identity instanceof BrowserIDManager);
> +
> +  Service.serverURL = "https://localhost/";

I think this will cause problems on our Mac machines if there's not actually a webserver running; bug 1010322 fixes non-existent localhost URLs in other bits of the testsuite.  Setting up a dummy HTTPS webserver might be difficult, though...maybe disable the test on Mac?

I can push to try to see if this instance of localhost works, but it'll be a while before I get those results.
Attachment #8422247 - Flags: feedback?(nfroyd) → feedback+
We also crash here:

2:06:42  WARNING -  TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\xpcshell\tests\services\sync\tests\unit\test_service_changePassword.js | test failed (with xpcshell return code: -2147483645), see following log:
12:06:42     INFO -  >>>>>>>
12:06:42     INFO -  [4728] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file c:\\builds\\moz2_slave\\try-w32-d-00000000000000000000\\build\\toolkit\\crashreporter\\nsExceptionHandler.cpp, line 2158
12:06:42     INFO -  System JS : WARNING resource://gre/modules/Preferences.jsm:378 - mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create
12:06:42     INFO -  [4728] WARNING: This method is lossy. Use GetCanonicalPath !: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/xpcom/io/nsLocalFileWin.cpp, line 3348
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test MAIN run_test pending (1)
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test run_next_test 0 pending (2)
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test MAIN run_test finished (2)
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | running event loop
12:06:42     INFO -  TEST-INFO | C:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_service_changePassword.js | Starting test_change_password
12:06:42     INFO -  TEST-INFO | (xpcshell/head.js) | test test_change_password pending (2)
12:06:42     INFO -  Sync.Identity	INFO	Username changed. Removing stored credentials.
12:06:42     INFO -  Sync.Identity	INFO	Basic password has no value. Removing.
12:06:42     INFO -  Sync.Identity	INFO	Sync Key has no value. Deleting.
12:06:42     INFO -  Sync.Identity	INFO	Basic password being updated.
12:06:42     INFO -  Sync.Identity	INFO	Sync Key has no value. Deleting.
12:06:42     INFO -  changePassword() returns false for a network error, the password won't change.
12:06:42     INFO -  Services.Common.RESTRequest	TRACE	HTTP Header authorization: ***** (suppressed)
12:06:42     INFO -  Services.Common.RESTRequest	DEBUG	POST Length: 11
12:06:42     INFO -  Services.Common.RESTRequest	TRACE	POST Body: ILoveJane83
12:06:42     INFO -  System JS : WARNING resource://testing-common/services-common/logging.js:30 - mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create
12:06:42     INFO -  [4728] WARNING: Non-local hostname auth.services.mozilla.com resolved: 1725560127:0: file c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/netwerk/dns/nsHostResolver.cpp, line 1193
12:06:42     INFO -  Sync.Tracker.Clients	WARN	Changed IDs file clients contains non-object value.
12:06:42     INFO -  Sync.Tracker.Bookmarks	WARN	Changed IDs file bookmarks contains non-object value.
12:06:42     INFO -  Sync.Tracker.Forms	WARN	Changed IDs file forms contains non-object value.
12:06:42     INFO -  Sync.Tracker.History	WARN	Changed IDs file history contains non-object value.
12:06:42     INFO -  Sync.Tracker.Passwords	WARN	Changed IDs file passwords contains non-object value.
12:06:42     INFO -  Sync.Tracker.Addons	WARN	Changed IDs file addons contains non-object value.
12:06:42     INFO -  BAD CONNECT: connecting to auth.services.mozilla.com 0
Fix for test_service_changePassword - but note this will also be hitting a non-existent server on localhost as the test starts (this test later creates a server on that port, but the initial test is before the server starts and the comments imply it is specifically checking a network error case)

I'm holding off requesting review until I get clarification this doesn't cause the other issues around non-existing servers on localhost that we briefly discussed on IRC - please let me know when you are confident this is or isn't a problem, and I'll get rnewman to review.
Attachment #8422247 - Attachment is obsolete: true
Component: Server: Firefox Accounts → Firefox Sync: Backend
Flags: firefox-backlog+
(In reply to Mark Hammond [:markh] from comment #6)
> I'm holding off requesting review until I get clarification this doesn't
> cause the other issues around non-existing servers on localhost that we
> briefly discussed on IRC - please let me know when you are confident this is
> or isn't a problem, and I'll get rnewman to review.

I haven't gotten a clean run of xpcshell tests on OSX yet, but I do see in the logs that even with this patch, the relevant tests timeout when being run in parallel:

https://tbpl.mozilla.org/php/getParsedLog.php?id=40731121&tree=Try&full=1

This is kind of annoying.  Can you spin up dummy HTTP servers inside the tests instead and just point the tests at those?
Flags: needinfo?(mhammond)
(In reply to Nathan Froyd (:froydnj) from comment #7)

> This is kind of annoying.  Can you spin up dummy HTTP servers inside the
> tests instead and just point the tests at those?

We could, yes.  Some sync tests already do this, but the sync test infra is somewhat opaque, so not a trivial change.
Flags: needinfo?(mhammond)
Whiteboard: p=5
To clarify this a little bit for the record: the Sync tests themselves all talk to local servers spun up inside tests.

These hits are for the new FxA parts that surround Sync.

See services/sync/tests/unit/head_http_server.js for the server stuff, and services/sync/modules-testing/utils.js (particularly SyncTestingInfrastructure) for how Sync is configured to point to a local test server.

Taking a quick look, test_fxa_startOver seems to be missing some of the configuration steps that SyncTestingInfrastructure applies.
I got some clean OS X xpcshell runs with the patch in this bug and some changes that I needed for bug 101153:

https://tbpl.mozilla.org/?tree=Try&rev=3c1e8a023a01

Hooray!  Mark, can you select an appropriate reviewer for this patch?
Flags: needinfo?(mhammond)
Comment on attachment 8423527 [details] [diff] [review]
0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

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

A couple of tweaks to prevent the sync xpcshell tests hitting the external network.
Attachment #8423527 - Flags: review?(rnewman)
Flags: needinfo?(mhammond)
Comment on attachment 8423527 [details] [diff] [review]
0001-Bug-995599-prevent-services-tests-hitting-the-networ.patch

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

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +168,5 @@
>    do_check_eq(result.assertion, credentials.assertion);
>    do_check_eq(result.kB, credentials.kB);
>  
>    // sign out
> +  yield account.signOut(true);

Gentle preference for

  let localOnly = true;
  yield account.signOut(localOnly);

Boolean arguments ftl.
Attachment #8423527 - Flags: review?(rnewman) → review+
thanks!  Pushed with suggested change:

https://hg.mozilla.org/integration/fx-team/rev/eb8e390f34fc

Nathan, can you please mark as verified when you are satisfied it is?
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Flags: needinfo?(nfroyd)
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: p=5 → p=5 [qa+]
https://hg.mozilla.org/mozilla-central/rev/eb8e390f34fc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Whiteboard: p=5 [qa+] → p=5 s=33.1 [qa+]
Hi Florin, can a contact be assigned for QA verification.
Flags: needinfo?(florin.mezei)
This doesn't need QA verification; it is a test-only fix.  The fix works in testing on Try.
Status: RESOLVED → VERIFIED
Flags: needinfo?(nfroyd)
Flags: needinfo?(florin.mezei)
Whiteboard: p=5 s=33.1 [qa+] → p=5 s=33.1 [qa-]
This patch was backported to b2g30 in comment 15, but it appears that we're still hitting problems in test_accounts.js with bug 995417 applied:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42378299&tree=Try

Any chance you can help with this Richard since Mark is away? :)
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/c6e856383007

is the patch that added localOnly. I can't imagine that Mark's fix would work without that patch.

Perhaps check that Bug 994934 is on b2g30?
Flags: needinfo?(rnewman)
Aha! Thanks :)
Turns out FxA is disabled on B2G v1.4, so I got spenrose's blessing to just disable the FxA tests on b2g30.

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/d2607c7b727e
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.