Remove support for external resources over cellular data

RESOLVED FIXED

Status

Firefox OS
Gaia::UI Tests
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: davehunt, Assigned: zac)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

46 bytes, text/x-github-pull-request
Martijn Wargers (zombie)
: review+
viorela
: review+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
This is something Zac proposed a while back but I defended the ability for people running the tests to be able to execute tests that use external resources when they don't have a WiFi connection.

Given this increases the effort when writing tests that use hosted resources, and that we're not aware of anyone needing to run the tests under these conditions I have come around to Zac's suggestion.

Unless a test explicitly needs to access a resource over cellular data, let's make any tests that use hosted resources fail early when run on a device without WiFi. A suitable failure message such as "Test requires resources from local server. Please run on a device with WiFi available or an emulated device with access to the machine running the tests."
Blocks: 989562
Assignee: nobody → martijn.martijn
Created attachment 8506865 [details] [review]
1084306

Like this?
test_homescreen_launch_app.py and test_homescreen_delete_app.py also need to be changed, but I'll do that as part of bug 989562.
Attachment #8506865 - Flags: review?(dave.hunt)
(Reporter)

Comment 2

4 years ago
Comment on attachment 8506865 [details] [review]
1084306

No, this checks that the machine running the tests can reach an http server running on the same machine. We really need to check if the target application (which may be remote) can reach the http server.

I wouldn't concern yourself over this, and just use the assumption we already have - if the device is not desktop and does not have wifi then it would use cellular data. Improving this to perform some sort of network check from the remote device would be great, but should be covered by a separate bug.
Attachment #8506865 - Flags: review?(dave.hunt) → review-
Comment on attachment 8506865 [details] [review]
1084306

I updated the pull request.
Attachment #8506865 - Flags: review- → review?(dave.hunt)
(Assignee)

Comment 4

4 years ago
I think this needs to be a quite more thorough pull request. 

IMO (but I am open to changes) we should remove test_connect_to_network entirely and replace the calls to that method with the method to connect to the exact connection we need for that test case.

This won't resolve the issue that someone might connect the device to a WiFi network that's different to that of the test runner's host computer. We will have to resolve that in a different way. Perhaps a simple HEAD request to the Marionette server and skip the test if it does not return.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1054307
(Reporter)

Comment 6

4 years ago
(In reply to Zac C (:zac) from comment #4)
> I think this needs to be a quite more thorough pull request. 
> 
> IMO (but I am open to changes) we should remove test_connect_to_network
> entirely and replace the calls to that method with the method to connect to
> the exact connection we need for that test case.

I'm okay with this suggestion.

> This won't resolve the issue that someone might connect the device to a WiFi
> network that's different to that of the test runner's host computer. We will
> have to resolve that in a different way. Perhaps a simple HEAD request to
> the Marionette server and skip the test if it does not return.

This is a separate bug (not sure if it's on file). A HEAD request would probably be fine but it would need to come from the device, not the machine running the tests.
(Reporter)

Comment 7

4 years ago
Comment on attachment 8506865 [details] [review]
1084306

Removing review request based on Zac's suggestion to remove the connect_to_network method (and all references).
Attachment #8506865 - Flags: review?(dave.hunt)
(Assignee)

Comment 8

4 years ago
(In reply to Dave Hunt (:davehunt) from comment #6)
> A HEAD request would
> probably be fine but it would need to come from the device, not the machine
> running the tests.

Of course, yes!
Created attachment 8509588 [details] [review]
1084306_v3

The test_connect_to_network call is only used for the unit test:
http://mxr.mozilla.org/mozilla-central/search?string=test_connect_to_network
http://mxr.mozilla.org/gaia/search?string=test_connect_to_network
Attachment #8506865 - Attachment is obsolete: true
Attachment #8509588 - Flags: review?(zcampbell)
(Reporter)

Comment 10

4 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #9)
> Created attachment 8509588 [details] [review]
> 1084306_v3
> 
> The test_connect_to_network call is only used for the unit test:
> http://mxr.mozilla.org/mozilla-central/search?string=test_connect_to_network
> http://mxr.mozilla.org/gaia/search?string=test_connect_to_network

You should search for connect_to_network not test_connect_to_network.
(Assignee)

Comment 11

4 years ago
Comment on attachment 8509588 [details] [review]
1084306_v3

Yes as Dave suggested, what we want to do is remove all connect_to_network() and replace them with connect_to_cell_data() or connect_to_local_network() .

Apologies my comment #4 was incorrect, I just meant remove the method. Sorry about that :(
Attachment #8509588 - Flags: review?(zcampbell) → review-
(Assignee)

Comment 12

4 years ago
Going to take this from Martijn (with his permission of course!)
Assignee: martijn.martijn → zcampbell
(Assignee)

Comment 13

4 years ago
Created attachment 8514410 [details] [review]
github pr
Attachment #8509588 - Attachment is obsolete: true
Attachment #8514410 - Flags: review?(viorela.ioia)
Attachment #8514410 - Flags: review?(robert.chira)
Attachment #8514410 - Flags: review?(martijn.martijn)
Comment on attachment 8514410 [details] [review]
github pr

Thanks! Looks good. I haven't tested it locally, but that's what you do with the adhoc test, anyway.
Attachment #8514410 - Flags: review?(martijn.martijn) → review+
Zac, looks good so far, but please address Viorela's comment from the PR
(Assignee)

Comment 17

4 years ago
(In reply to Robert Chira [:RobertC] from comment #16)
> Zac, looks good so far, but please address Viorela's comment from the PR

Thanks, that test wasn't merged when I forked yesterday :)

Updated the PR.
Attachment #8514410 - Flags: review?(robert.chira)
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.