Closed Bug 1095097 Opened 7 years ago Closed 7 years ago

Disable access during integration tests


(Firefox OS Graveyard ::, defect)

Not set


(Not tracked)



(Reporter: daleharvey, Assigned: daleharvey)



(Whiteboard: [systemsfe])


(1 file, 1 obsolete file)

Our integration tests die with MOZ_DISABLE_NONLOCAL_CONNECTIONS, is a big reason for that, a lot of the tests dont actually need interactions and the ones that do should be starting a local server, so this may be accidental access

  b2g-desktop stderr +3ms FATAL ERROR: Non-local network connections are disabled and a connection attempt to ( was made.
You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.

(this was with apps/verticalhome/test/marionette/app_appcache_pending_test.js)
Blocks: 1030045
Kevin, you did a bunch of the mocking, is there an easy way to disable the provider, we can try that and see how many tests survive, the rest that actually need a connection will have to be disabled and fixed one by one
Flags: needinfo?(kgrandon)
Dale - can you clarify here? Our tests need to be able to use standard things like SystemXHR to reach out over the internet, but it's ok if they fail. Most of our tests do not need intereactions, and I don't think this test does? I think that we typically just make standard internet connections in the background, and that these connections are not important for the test.

Can we make this log a warning instead of error? Basically not make the test die?

I don't think it's feasible to write our tests in a way to mock *every* http request, so we may need to talk to the sheriffs about this.
Flags: needinfo?(kgrandon) → needinfo?(dale)
Clarified in
Flags: needinfo?(dale)
Let's see what this does on try. Should significantly reduce the connection count.
This doesnt seem to / shouldnt effect any functionality, the url gets picked up by settings only, which is defined in so it gets rid of a possible confusing duplicate (someone changing the setting vs the code etc)

MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 NODE_DEBUG=b2g-desktop REPORTER=mocha-tbpl-reporter TEST_FILES=apps/verticalhome/test/marionette/app_appcache_pending_test.js make test-integration

is what I have been using to test, will crash without the patch, passes with
Assignee: nobody → dale
Attachment #8518891 - Flags: review?(kgrandon)
Comment on attachment 8518645 [details] [review]
Pull Request - default to localhost for connections

Sounds good, thanks Dale.
Attachment #8518645 - Attachment is obsolete: true
Actually thinking that having the url being set to null and having the promise rejected would be the more correct thing than sending requests to an arbitrary url, but will wait to hear what you think
(url set to null in the settings override for the tests is what I meant btw)
Comment on attachment 8518891 [details] [review]

It does seem like we have a single test which is now being perma-failed due to this change,  TEST-UNEXPECTED-FAIL | null | Vertical - Search Search notification .

Please take a look and re-flag me. Thanks.
Attachment #8518891 - Flags: review?(kgrandon)
Comment on attachment 8518891 [details] [review]

We need to add an mock to properly test that as the search notification only shows when remote results are loaded, I added it to be skipped to the manifest and filed a follow up to fix
Attachment #8518891 - Flags: review?(kgrandon)
Let's just add the mock server. We already have an server, so I don't think we should disable the test.
It takes about ~5 lines of boilerplate to setup the server:

We can make this easier too if needed.
ok will take a look and do that patch, but in general my plan was to fix large problems (marketplace access etc) disable individual tests when needed then revisit / assign the disabled bugs when the tests are visible, if we try to fix every test as we go its just going to go around the same loop. 2.1 has already gone permafail since I have been working on these and it looks like a bunch more intermittents are on master that have landed that will need another few run throughs once this bunch of bugs gets fixed
Comment on attachment 8518891 [details] [review]

Clearing review for now. My preference is to add the mock server instead of disabling the test. Feel free to assign the bug to me if you need some help or don't have time for it.
Attachment #8518891 - Flags: review?(kgrandon)
Whiteboard: [systemsfe]
Comment on attachment 8518891 [details] [review]

Updated to use mock
Attachment #8518891 - Flags: review?(kgrandon)
Comment on attachment 8518891 [details] [review]

Seems fine to me, thanks. I left one comment on github and noticed that there were a few errors on the try run, so I ran a few retriggers. Let's see if we get a green here.
Attachment #8518891 - Flags: review?(kgrandon) → review+
2 of the failures were legitimate, since the mock was returning results it changed the behaviour of the tests, fixed those, couldnt get the others reproduced locally so triggered another run with the fixes in to see if it goes green
New run is green
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.