1015901 - Wifi on emulator doesn't remember networks

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Henry Chang [:hchang]]

Bug 991025 enables wifi on emulator. But the lack of "update_config=1" in the template wpa_supplicant.conf makes it unable to remember APs that we have ever connected. We should add "update_config=1" to [1]

[1] https://github.com/mozilla-b2g/device_generic_goldfish/blob/master/wifi/wpa_supplicant.conf

Comment 1

[Henry Chang [:hchang]]

Attachment: https://github.com/mozilla-b2g/device_generic_goldfish/pull/14/files

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8429722 [details]
https://github.com/mozilla-b2g/device_generic_goldfish/pull/14/files

Test cases?

Comment 3

[Henry Chang [:hchang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> Comment on attachment 8429722 [details]
> https://github.com/mozilla-b2g/device_generic_goldfish/pull/14/files
> 
> Test cases?

There will a test case based on http://hg.mozilla.org/mozilla-central/file/83ad75d867af/dom/wifi/test/marionette/head.js

Probably need to test with |dontConnect| flag [1] as well.

[1] http://hg.mozilla.org/mozilla-central/file/83ad75d867af/dom/webidl/MozWifiManager.webidl#l91

Comment 4

[Henry Chang [:hchang]]

Attachment: Test cases

Comment 5

[Henry Chang [:hchang]]

Try run result: https://tbpl.mozilla.org/?tree=Try&rev=b7c442017b69
with the attached patch and PR applied.

Comment 6

[Henry Chang [:hchang]]

Comment on attachment 8433258 [details] [diff] [review]
Test cases

Hi vicamo, I attached the test cases for the modification of the template wpa_supplicant.conf. The main idea is, without the line "update_config=1" in the template wpa_supplicant.conf, the config file will never be updated to remember the ever connected network information. As a result, if we get connected to certain AP and re-enable Wifi, it will not automatically connect to the AP we connected before. This "re-enable" scenario is what "test_wifi_auto_connect.js" wants to test. 

Besides, we can also test "update_config=1" in different aspect: MozWifiManager::getKnwonNetworks. |getKnownNetworks| internally requests wpa_supplicant to parse the config file and reports the remembered networks. "test_wifi_associate_wo_connect.js" attempts to associate but not connect to certain AP and check if |getKnownNetworks| returns that AP. We have to use the flag |dontConnect| because |getKnownNetworks| will return the remembered networks as well as the active networks.

Thanks!

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8433258 [details] [diff] [review]
Test cases

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

::: dom/wifi/test/marionette/test_wifi_associate_wo_connect.js
@@ +46,5 @@
> +        return;
> +      }
> +      throw 'Got unexpected connected event!';
> +    });
> +}

I will remove the flag |timeout| and throw a boolean to indicate if this promise is resolved or not.

Comment 8

[Henry Chang [:hchang]]

Attachment: Test cases V2

Vicamo, if you haven't started reviewing the previous one, please change to review V2 which addressed the removal of the ugly flag I spoke to you earlier. Thanks!

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8433258 [details] [diff] [review]
Test cases

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

::: dom/wifi/test/marionette/head.js
@@ +248,5 @@
> +
> +  /**
> +   * Forget all known networks.
> +   *
> +   * Resolve when we successfully forget all the known network; 

nit: trailing sp

::: dom/wifi/test/marionette/test_wifi_associate_wo_connect.js
@@ +64,5 @@
> + *        MozWifiNetwork object.
> + *
> + * @return A deferred promise.
> + */
> +function associateButDontConnect(aNetwork) {

I think you can reuse 'testAssociate' and simplify this as:

  aNetwork.dontConnect = true;

  let promises = [];
  promises.push(waitForTimeout(10 * 1000)
                .then(() => { throw 'timeout'; }));

  promises.push(gTestSuite.testAssociate(aNetwork));

  return Promise.all(promises)
    .then(() => { throw 'unexpected state'; },
          function(aReason) {
      is(typeof aReason, 'string', 'typeof aReason');
      is(aReason, 'timeout', aReason);
    });

Comment 10

[Henry Chang [:hchang]]

Attachment: Test cases V3

1) Remove trailing spaces.
2) Simplify |associateButDontConnect|
3) Use is(xxx, xxx) instead of throwing exceptions.

Comment 11

[Henry Chang [:hchang]]

Comment on attachment 8434639 [details] [diff] [review]
Test cases V3

hi Vincent, I added two wifi test cases relevant to "update_config=1". Please refer to Comment 6 for further information. Thanks!

Comment 12

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #11)
> Comment on attachment 8434639 [details] [diff] [review]
> Test cases V3
> 
> hi Vincent, I added two wifi test cases relevant to "update_config=1".
> Please refer to Comment 6 for further information. Thanks!

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c0bd8c6ec49b

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8434639 [details] [diff] [review]
Test cases V3

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

The patches looks good to me, just a couple of suggestion.

::: dom/wifi/test/marionette/head.js
@@ +383,5 @@
> +    // Then we do the association.
> +    let request = wifiManager.associate(aNetwork);
> +    promises.push(wrapDomRequestAsPromise(request));
> +
> +    return Promise.all(promises);

I think we should add a timeout here? Like what you have done in associateButDontConnect().

::: dom/wifi/test/marionette/test_wifi_associate_wo_connect.js
@@ +35,5 @@
> +          function(aReason) {
> +      is(typeof aReason, 'string', 'typeof aReason');
> +      is(aReason, 'timeout', aReason);
> +    });
> +}

Do you want to move this function to head.js as well? Since you have moved testAssociate()?

Comment 14

[Henry Chang [:hchang]]

(In reply to Vincent Chang[:vchang] from comment #13)
> Comment on attachment 8434639 [details] [diff] [review]
> Test cases V3
> 
> Review of attachment 8434639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patches looks good to me, just a couple of suggestion.
> 
> ::: dom/wifi/test/marionette/head.js
> @@ +383,5 @@
> > +    // Then we do the association.
> > +    let request = wifiManager.associate(aNetwork);
> > +    promises.push(wrapDomRequestAsPromise(request));
> > +
> > +    return Promise.all(promises);
> 
> I think we should add a timeout here? Like what you have done in
> associateButDontConnect().
> 
> ::: dom/wifi/test/marionette/test_wifi_associate_wo_connect.js
> @@ +35,5 @@
> > +          function(aReason) {
> > +      is(typeof aReason, 'string', 'typeof aReason');
> > +      is(aReason, 'timeout', aReason);
> > +    });
> > +}
> 
> Do you want to move this function to head.js as well? Since you have moved
> testAssociate()?

Vincent, 

Thanks for the review! 

Vicamo, 

Since all the stuff have been r+, could you please help merge the PR so that I can
commit the test case to gecko afterward? Thanks!

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

https://github.com/mozilla-b2g/device_generic_goldfish/commit/3cae7eee1f7f00ba1eb53c733a8779f345055c6e
https://hg.mozilla.org/integration/b2g-inbound/rev/553b7aecee5c

Comment 16

[Henry Chang [:hchang]]

Vicamo, thanks!

The try run based on merged PR and test cases:

https://tbpl.mozilla.org/?tree=Try&rev=565c7b43c16c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/32c4e06960f2

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32c4e06960f2