997090 - Basic wifi test case on ICS emulator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Henry Chang [:hchang]]

This bug is for the very basic wifi test case on the basis of Bug 991025. The test case should include enable wifi, scan, associate, disable wifi.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug997090_wifi_test_case.patch

Comment 2

[Henry Chang [:hchang]]

Hi wifi guys, this is the basic wifi test case based on Bug 991025. If you are interested in trying this out, you can use the following one-line command to checkout-build-run this test case:

git clone https://github.com/mozilla-b2g/B2G b2g-emu-wifi && cd b2g-emu-wifi && GITREPO=git://github.com/elefant/b2g-manifest BRANCH=dev/emu-wifi ./config.sh emulator && ./build.sh && ./mach marionette-webapi gecko/dom/wifi/test/marionette/test_wifi.js

Comment 3

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

I can't admire more than typing this comment on my knees. <(_ _)>

It seems network can't be stringified in Gaia, but that's irrelevant to this master piece.

Comment 4

[Henry Chang [:hchang]]

(In reply to Chuck Lee [:chucklee] from comment #3)
> I can't admire more than typing this comment on my knees. <(_ _)>
> 
> It seems network can't be stringified in Gaia, but that's irrelevant to this
> master piece.

It used to be okay >"< Checking this out...

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Thanks Henry, it's really a giant step for wifi module.

Comment 6

[Ken Chang[:kenkai]]

Please this into backlog.

Comment 7

[Henry Chang [:hchang]]

Attachment: wifi_test_cases.patch

Updated test cases: Refactor with Promise.

Comment 8

[Henry Chang [:hchang]]

Attachment: TestCase.patch

Comment 9

[Henry Chang [:hchang]]

Attachment: TestCase.patch

Comment 10

[Henry Chang [:hchang]]

Finally passed my local full marionette test. Testing on try:

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

Comment 11

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8412442 [details] [diff] [review]
TestCase.patch

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

I am not familiar with promise, so just correct me if my understanding is wrong.

::: dom/wifi/test/marionette/head.js
@@ +62,5 @@
> +  // Ensure wifi is enabled.
> +  function ensureEnabled() {
> +    if (wifiManager.enabled) {
> +      log('Already enabled');
> +      return;

It seems that we expect |ensureEnabled()| return a promise, maybe we need defer promise and resolve here?
Or move the check into |testEnabled()| so we always returns a promise?

@@ +73,5 @@
> +    });
> +  }
> +
> +  // Ensure wifi is disabled.
> +  function ensureDisabled() {

Same as |ensureEnabled()|;

@@ +94,5 @@
> +    wifiManager.ondisabled = function() {
> +      ok(true, 'wifiManager.ondisabled');
> +      deferred.resolve(true);
> +    };
> +

We should check if |wifi.enabled| is already false, because |wifiManager.ondisabled| won't be triggered if we disable wifi while it is already disabled.

@@ +113,5 @@
> +    wifiManager.onenabled = function() {
> +      ok(true, 'wifiManager.onenabled');
> +      deferred.resolve(true);
> +    };
> +

We should check if |wifi.enabled| is already true, because |wifiManager.onenabled| won't be triggered if we enable wifi while it is already enabled.

@@ +119,5 @@
> +      'wifi.enabled': true
> +    }).onerror = function() {
> +      ok(false, 'Unable to enable wifi');
> +      deferred.resolve(false);
> +    };

wifiManager.ondisabled will be triggered when fail to enable wifi.
I think we should put |deffered.resolve(false)| in this callback, like the onerror above.

::: dom/wifi/test/marionette/test_wifi_associate.js
@@ +4,5 @@
> +let HOSTAPD_CONFIG_LIST = [
> +  {
> +    ssid: 'ap0',
> +  },
> +

nit: no new line here.

::: dom/wifi/test/marionette/test_wifi_enable.js
@@ +3,5 @@
> +
> +gTestSuite.doTest(function() {
> +  return Promise.resolve()
> +    .then(gTestSuite.ensureDisabled)
> +    .then(gTestSuite.testEnable);

Why not use |ensureEnabled| here?

Comment 12

[Henry Chang [:hchang]]

Thanks your review!
Please see the inline reply:

(In reply to Chuck Lee [:chucklee] from comment #11)
> Comment on attachment 8412442 [details] [diff] [review]
> TestCase.patch
> 
> Review of attachment 8412442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not familiar with promise, so just correct me if my understanding is
> wrong.
> 

The general idea of ensureEn(Dis)able/testEn(Dis)able is,
ensureEn(Dis)able is used to ensure the wifi enabled while
testEn(Dis)able is used to definitely request the internal
wifi machinery to enable/disable.

> ::: dom/wifi/test/marionette/head.js
> @@ +62,5 @@
> > +  // Ensure wifi is enabled.
> > +  function ensureEnabled() {
> > +    if (wifiManager.enabled) {
> > +      log('Already enabled');
> > +      return;
> 
> It seems that we expect |ensureEnabled()| return a promise, maybe we need
> defer promise and resolve here?
> Or move the check into |testEnabled()| so we always returns a promise?
> 

The test suite functions are designed to be a resolver of Promise.
So you can put them in Promise.then(). Promise will schedule the next task
according to the returned object: If the returned object is then-able (i.e.
 has |then| function), then the next resolver will be called whenever the returned
Promise is resolved. Otherwise, the next resolver will be call immediately.

As a result, you can put this function to Promise.then() with no worries.

http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise-backend.js#655

> @@ +73,5 @@
> > +    });
> > +  }
> > +
> > +  // Ensure wifi is disabled.
> > +  function ensureDisabled() {
> 
> Same as |ensureEnabled()|;
> 
> @@ +94,5 @@
> > +    wifiManager.ondisabled = function() {
> > +      ok(true, 'wifiManager.ondisabled');
> > +      deferred.resolve(true);
> > +    };
> > +
> 
> We should check if |wifi.enabled| is already false, because
> |wifiManager.ondisabled| won't be triggered if we disable wifi while it is
> already disabled.
> 

The pre-condition of this function is wifi enabled. Maybe I should add this pre-condition
to this function.

> @@ +113,5 @@
> > +    wifiManager.onenabled = function() {
> > +      ok(true, 'wifiManager.onenabled');
> > +      deferred.resolve(true);
> > +    };
> > +
> 
> We should check if |wifi.enabled| is already true, because
> |wifiManager.onenabled| won't be triggered if we enable wifi while it is
> already enabled.
> 
> @@ +119,5 @@
> > +      'wifi.enabled': true
> > +    }).onerror = function() {
> > +      ok(false, 'Unable to enable wifi');
> > +      deferred.resolve(false);
> > +    };
> 
> wifiManager.ondisabled will be triggered when fail to enable wifi.
> I think we should put |deffered.resolve(false)| in this callback, like the
> onerror above.
> 

Got it.

> ::: dom/wifi/test/marionette/test_wifi_associate.js
> @@ +4,5 @@
> > +let HOSTAPD_CONFIG_LIST = [
> > +  {
> > +    ssid: 'ap0',
> > +  },
> > +
> 
> nit: no new line here.

Got it!

> 
> ::: dom/wifi/test/marionette/test_wifi_enable.js
> @@ +3,5 @@
> > +
> > +gTestSuite.doTest(function() {
> > +  return Promise.resolve()
> > +    .then(gTestSuite.ensureDisabled)
> > +    .then(gTestSuite.testEnable);
> 
> Why not use |ensureEnabled| here?

It's because ensureEn(Dis)abled function is not guaranteed to actually
request to enable. ensureEnable will request to enable only when
wifi is not enabled. testEn(Dis)able is the function we certainly know
the the enable/disable task in requested.

I will put my idea to head.js to avoid any confusion of those functions.
Thanks!

Comment 13

[Henry Chang [:hchang]]

Attachment: Test case V2

Comment 14

[Henry Chang [:hchang]]

Try result with patch V2: https://tbpl.mozilla.org/?tree=Try&rev=231550e1bc4a

Comment 15

[Henry Chang [:hchang]]

Comment on attachment 8418565 [details] [diff] [review]
Test case V2

Vicamo, could you give us some feedback regarding the test cases and the use of Promise? Thanks :)

Comment 16

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

Comment on attachment 8418565 [details] [diff] [review]
Test case V2

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

::: dom/wifi/test/marionette/head.js
@@ +1,1 @@
> +let Promise = SpecialPowers.Cu.import('resource://gre/modules/Promise.jsm').Promise;

nit: missing license declaration.

@@ +1,3 @@
> +let Promise = SpecialPowers.Cu.import('resource://gre/modules/Promise.jsm').Promise;
> +
> +let HOSTAPD_CONFIG_PATH = '/data/misc/wifi/hostapd/';

nit: that's probably not supposed to be mutable. Let's declare it as a constant instead.

@@ +21,5 @@
> +  function getProcessDetail(aProcessName) {
> +    let deferred = Promise.defer();
> +    let detail = [];
> +
> +    runEmulatorShell(['ps'], function(processes) {

I'd like to have a generic, promise-based API wrapping the runEmulatorShell() call.  Besides, like it's near relative, runEmulatorCmd, this call has to be ref-counted, and you must guarantee that there is no pending callbacks at the time you calls finish().  See http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#14 for reference implementation.

@@ +29,5 @@
> +      // root      1     0     284    204   c009e6c4 0000deb4 S /init
> +      // root      2     0     0      0     c0052c64 00000000 S kthreadd
> +      // root      3     2     0      0     c0044978 00000000 S ksoftirqd/0
> +      //
> +      for (let i = 0; i < processes.length; i++) {

nit: shouldn't we skip at least the first row?

@@ +31,5 @@
> +      // root      3     2     0      0     c0044978 00000000 S ksoftirqd/0
> +      //
> +      for (let i = 0; i < processes.length; i++) {
> +        let tokens = processes[i].split(/\s+/);
> +        let pname = tokens[tokens.length - 1];

Please use |tokens[8]| instead because we may have empty process names.  Actually, we may also have names containing white spaces, but I'm fine with this for now.

@@ +50,5 @@
> +    let deferred = Promise.defer();
> +
> +    // The permission required by this test case is wifi-manage and settings-write.
> +    SpecialPowers.pushPermissions([{ 'type': 'wifi-manage', 'allow': 1, 'context': window.document }], function() {
> +      SpecialPowers.pushPermissions([{'type': 'settings-write', 'allow': true, 'context': document}], function() {

You may pass multiple entries to |SpecialPowers.pushPermissions| at once.

@@ +73,5 @@
> +    }
> +    return requestWifiEnable().then(success => {
> +      if (!success) {
> +        ok(false, 'ensureWifiEnabled failed');
> +        finish(); // Just finish the test if failed to enable wifi.

You're still in the promise execution chain.  Calling finish() at this time may result in various problem because followed promises are still ongoing.  You should reject in requestWifiEnable() when it fails to enable WiFi.

@@ +87,5 @@
> +    }
> +    return requestWifiDisable().then(success => {
> +      if (!success) {
> +        ok(false, 'ensureWifiDisabled failed');
> +        finish(); // Just finish the test if failed to enable wifi.

ditto.

@@ +104,5 @@
> +      ok(true, 'wifiManager.onenabled');
> +      deferred.resolve(true);
> +    };
> +
> +    window.navigator.mozSettings.createLock().set({

This file, header.js, is supposed to provide convenient utility functions for WiFi related test cases.  I would rather split everything into some atom calls and composite some of them into one complex function if necessary.  For example:

  // Wait for arbitrary named event.  Never reject.
  function waitForWifiManagerEvent(aEventName) { ... }

  // mozSettings setter.  Resolve if success, reject otherwise.
  function setSettings(aSettings) {}

  function setWifiEnabled(aEnabled) {
    let promises = [];
    promises.push(waitForWifiManagerEvent(aEnabled ? "enabled" : "disabled"));
    promises.push(setSettings({'wifi.enabled': aEnabled}));
    return Promise.all(promises);
  }

@@ +127,5 @@
> +    window.navigator.mozSettings.createLock().set({
> +      'wifi.enabled': false
> +    }).onerror = function() {
> +      ok(false, 'Unable to disable wifi');
> +      deferred.resolve(false);

ditto.

@@ +134,5 @@
> +    return deferred.promise;
> +  }
> +
> +  // Start a list of hostapd processes by given hostapd configs.
> +  function startHostapds(aConfigList) {

I'd like to split this function into smaller pieces:

  // Return a merged hostapd config object.
  function mergeHostapdConfig(aAdditionalAttrs) { ... }

  function startHostapd(aIndex, aConfig) {
    aConfig.interface = 'AP-' + aIndex;

    let configFileName = HOSTAPD_CONFIG_PATH + 'ap' + aIndex + '.conf';
    return writeHostapdConfFile(configFileName, aConfig)
      .then(() => runEmulatorShellSafe([hostapd', '-B', configFileName]));
  }

  function startMultipleHostapd(aConfigArray) {
    let promises = [];
    aConfigArray.map(function(aConfig, aIndex) {
      let mergedConfig = mergeHostapdConfig(aConfig);
      promises.push(startHostapd(aIndex, mergedConfig));
    });
    return Promise.all(promises);
  }

@@ +173,5 @@
> +    return p;
> +  }
> +
> +  // Kill all running hostapd processes.
> +  function killAllHostapd() {

return getProcessDetail('hostapd')
  .then(aProcesses => {
    let promises = [];
    aProcesses.map(function(aProcess) {
      promises.push(runEmulatorShellSafe(['kill', '-9', aProcess.pid]));
    });
    return Promise.all(promises);
  });

@@ +191,5 @@
> +
> +    return deferred.promise;
> +  }
> +
> +  function writeHostapdConfFile(aFileName, aConfig, aCallback) {

nit: use Promise as all other parts do.

@@ +201,5 @@
> +    }
> +    writeFile(aFileName, content, aCallback);
> +  }
> +
> +  function writeFile(aFileName, aContent, aCallback) {

nit: use Promise as all other parts do.

@@ +212,5 @@
> +    });
> +  }
> +
> +  // Start or stop an init.rc service.
> +  function serviceControl(aServiceName, aStart) {

nit: method name should always be an action, or a verb.  Use 'startStopInitService' or something similar.

@@ +216,5 @@
> +  function serviceControl(aServiceName, aStart) {
> +    let deferred = Promise.defer();
> +    let retryCnt = 10;
> +
> +    function isServiceRunning(aServiceName, aCallback) {

nit: use Promise as others do, please.  So you may have:

  function startStopInitService(aServiceName, aStart) {
    let retryCount = 10;

    return getInitServiceState(aServiceName)
      .then(function onresolve(aState) {
        if (aState != 'running') {
          return;
        }

        retryCount--;
        if (!retryCount) {
          throw 'Failed to ...';
        }

        return runEmulatorShell([(aStart ? 'start' : 'stop'), aServiceName])
          /*** I think there should be a small time delay. ***/
          .then(() => getInitServiceState(aServiceName))
          .then(onresolve);
      })
  }

@@ +240,5 @@
> +    return deferred.promise;
> +  }
> +
> +  // Start goldfish hostapd.
> +  function startStockHostapd(aCallback) {

function startStopStockHostapd(aStart) {
  return startStopInitService('goldfish-hostapd', aStart);
}

@@ +242,5 @@
> +
> +  // Start goldfish hostapd.
> +  function startStockHostapd(aCallback) {
> +    // There is a bug that it always fails to restart goldfish-hostapd
> +    // at the first time. So we have the second chance to restart it.

What's the problem?  Will a retry time delay in startStopInitService() help?

@@ +269,5 @@
> +  suite.startHostapds = startHostapds;
> +  suite.getProcessDetail = getProcessDetail;
> +  suite.killAllHostapd = killAllHostapd;
> +
> +  suite.doTest = function(aDoTest) {

I think you should at least provide two test case entrances.  One for normal AP-related cases, another for AP-unrelated ones like test_wifi_enable.js.

@@ +278,5 @@
> +        wifiManager = window.navigator.mozWifiManager;
> +        origEnabled = wifiManager.enabled;
> +      })
> +      .then(stopStockHostapd)
> +      .then(success => ok(success, 'Stop stock hostapd'))

Use Promise.reject() for something not accomplished, errors, and then catch that un-fulfilled promise at the end.  I'm expecting something like:

  .then(null, function onreject() {
    ok(false, 'rejected promise caught');
  })
  .then(cleanUp);

You should try to kill all hostapd, restart goldfish-hostapd, wait for pended runEmulatorShell callbacks, restore WiFi enabled state, and finally call to finish().

::: dom/wifi/test/marionette/test_wifi_associate.js
@@ +43,5 @@
> +      deferred.resolve(true);
> +    }
> +  };
> +
> +  let req = gWifiManager.associate(aNetwork);

You need http://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/tests/marionette/head.js#50

@@ +50,5 @@
> +    // Waiting for status changed event.
> +  };
> +  req.onerror = function() {
> +    ok(false, 'gWifiManager.associate');
> +    deferred.resolve(false);

Please use reject().

Comment 17

[Henry Chang [:hchang]]

Thanks for your pretty useful feedback! Please see my inline reply below:

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> Comment on attachment 8418565 [details] [diff] [review]
> Test case V2
> 
> Review of attachment 8418565 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +104,5 @@
> > +      ok(true, 'wifiManager.onenabled');
> > +      deferred.resolve(true);
> > +    };
> > +
> > +    window.navigator.mozSettings.createLock().set({
> 
> This file, header.js, is supposed to provide convenient utility functions
> for WiFi related test cases.  I would rather split everything into some atom
> calls and composite some of them into one complex function if necessary. 
> For example:
> 
>   // Wait for arbitrary named event.  Never reject.
>   function waitForWifiManagerEvent(aEventName) { ... }
> 
>   // mozSettings setter.  Resolve if success, reject otherwise.
>   function setSettings(aSettings) {}
> 
>   function setWifiEnabled(aEnabled) {
>     let promises = [];
>     promises.push(waitForWifiManagerEvent(aEnabled ? "enabled" :
> "disabled"));
>     promises.push(setSettings({'wifi.enabled': aEnabled}));
>     return Promise.all(promises);
>   }
> 

Are these atom calls supposed to be exposed or only internal use?
If they are only for internal use, is it worthy of wrapping them
to promise?

> @@ +134,5 @@
> > +    return deferred.promise;
> > +  }
> > +
> > +  // Start a list of hostapd processes by given hostapd configs.
> > +  function startHostapds(aConfigList) {
> 
> I'd like to split this function into smaller pieces:
> 
>   // Return a merged hostapd config object.
>   function mergeHostapdConfig(aAdditionalAttrs) { ... }
> 
>   function startHostapd(aIndex, aConfig) {
>     aConfig.interface = 'AP-' + aIndex;
> 
>     let configFileName = HOSTAPD_CONFIG_PATH + 'ap' + aIndex + '.conf';
>     return writeHostapdConfFile(configFileName, aConfig)
>       .then(() => runEmulatorShellSafe([hostapd', '-B', configFileName]));
>   }
> 
>   function startMultipleHostapd(aConfigArray) {
>     let promises = [];
>     aConfigArray.map(function(aConfig, aIndex) {
>       let mergedConfig = mergeHostapdConfig(aConfig);
>       promises.push(startHostapd(aIndex, mergedConfig));
>     });
>     return Promise.all(promises);
>   }
> 

The reason I didn't split it like you said is because they are simple, integral and
one-time-use operations. Probably |mergeHostapdConfig| can be extracted to a function or
be added more comments...

> @@ +173,5 @@
> > +    return p;
> > +  }
> > +
> > +  // Kill all running hostapd processes.
> > +  function killAllHostapd() {
> 
> return getProcessDetail('hostapd')
>   .then(aProcesses => {
>     let promises = [];
>     aProcesses.map(function(aProcess) {
>       promises.push(runEmulatorShellSafe(['kill', '-9', aProcess.pid]));
>     });
>     return Promise.all(promises);
>   });
> 

Promise.all() is awesome! I hope I knew it before.

> @@ +191,5 @@
> > +
> > +    return deferred.promise;
> > +  }
> > +
> > +  function writeHostapdConfFile(aFileName, aConfig, aCallback) {
> 
> nit: use Promise as all other parts do.
> 

I was also considering wrapping it to promise-base API but ended up with
not doing so because I think it's too low level and not a part of test suite
API to get too much benefit.

> function startStopStockHostapd(aStart) {
>   return startStopInitService('goldfish-hostapd', aStart);
> }> @@ +242,5 @@
> > +
> > +  // Start goldfish hostapd.
> > +  function startStockHostapd(aCallback) {
> > +    // There is a bug that it always fails to restart goldfish-hostapd
> > +    // at the first time. So we have the second chance to restart it.
> 
> What's the problem?  Will a retry time delay in startStopInitService() help?

We will always get a "socket already bind error" in the first run. (not sure if
it's due to hostapd not using "SO_REUSEADDR")
Maybe we are able to restart it successfully in the first run after stopping it for
a long while. (but I never tried it). Doing restart twice always works!

> 
> @@ +269,5 @@
> > +  suite.startHostapds = startHostapds;
> > +  suite.getProcessDetail = getProcessDetail;
> > +  suite.killAllHostapd = killAllHostapd;
> > +
> > +  suite.doTest = function(aDoTest) {
> 
> I think you should at least provide two test case entrances.  One for normal
> AP-related cases, another for AP-unrelated ones like test_wifi_enable.js.
> 

How will they look differently? Could you please explain more or is there existing
any example for reference? Thanks!

Comment 18

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8418565 [details] [diff] [review]
Test case V2

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

Drop the review for now since Vicamo has given the feedback. 
Please ask for review after addressing the review comments. :-)

Comment 19

[Henry Chang [:hchang]]

Attachment: Wifi Test Case V3

Comment 20

[Henry Chang [:hchang]]

Attachment: Test case V3

Comment 21

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

(In reply to Henry Chang [:henry] from comment #17)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> > Review of attachment 8418565 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +104,5 @@
> > > +      ok(true, 'wifiManager.onenabled');
> > > +      deferred.resolve(true);
> > > +    };
> > > +
> > > +    window.navigator.mozSettings.createLock().set({
> > 
> > This file, header.js, is supposed to provide convenient utility functions
> > for WiFi related test cases.  I would rather split everything into some atom
> > calls and composite some of them into one complex function if necessary. 
> > For example:
> > 
> >   // Wait for arbitrary named event.  Never reject.
> >   function waitForWifiManagerEvent(aEventName) { ... }
> > 
> >   // mozSettings setter.  Resolve if success, reject otherwise.
> >   function setSettings(aSettings) {}
> > 
> >   function setWifiEnabled(aEnabled) {
> >     let promises = [];
> >     promises.push(waitForWifiManagerEvent(aEnabled ? "enabled" :
> > "disabled"));
> >     promises.push(setSettings({'wifi.enabled': aEnabled}));
> >     return Promise.all(promises);
> >   }
> > 
> 
> Are these atom calls supposed to be exposed or only internal use?
> If they are only for internal use, is it worthy of wrapping them
> to promise?

They are convenient APIs for other test case files.

> > @@ +269,5 @@
> > > +  suite.startHostapds = startHostapds;
> > > +  suite.getProcessDetail = getProcessDetail;
> > > +  suite.killAllHostapd = killAllHostapd;
> > > +
> > > +  suite.doTest = function(aDoTest) {
> > 
> > I think you should at least provide two test case entrances.  One for normal
> > AP-related cases, another for AP-unrelated ones like test_wifi_enable.js.
> > 
> 
> How will they look differently? Could you please explain more or is there
> existing
> any example for reference? Thanks!

suite.doTest = function(aDoTest) {
  let origEnabled;
  return addRequiredPermissions()
    .then(() => {
      wifiManager = window.navigator.mozWifiManager;
      origEnabled = wifiManager.enabled;
    })
    .then(aDoTest)
    .then(() => origEnabled ? ensureWifiEnabled() : ensureWifiDisabled())
    .then(finish);
};

suite.doApTest = function(aDoTest) {
  return suite.doTest(function() {
    return stopStockHostapd()
      .then(aDoTest)
      .then(startStockHostapd);
  });
};

Comment 22

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

Comment on attachment 8421457 [details] [diff] [review]
Test case V3

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

::: dom/wifi/test/marionette/head.js
@@ +105,5 @@
> +        // root      2     0     0      0     c0052c64 00000000 S kthreadd
> +        // root      3     2     0      0     c0044978 00000000 S ksoftirqd/0
> +        //
> +        let detail = [];
> +        for (let i = 1 /*Skip the first line*/ ; i < processes.length; i++) {

I don't really like this inline comment.  We can have:

  // Skip the first line.
  processes.shift();

  let detail = [];
  for (let i = 0; i < processes.length; i++) { ... }

@@ +225,5 @@
> +      'wifi.enabled': true
> +    });
> +
> +    return wrapDomRequestAsPromise(request)
> +      .then(() => waitForWifiManagerEventOnce('enabled'));

Sorry, this can't be done in this way because it may introduce racing between event dispatching and listener registration per prior experience.  You should always make sure waitForWifiManagerEventOnce() is called __before__ you ever make the request.  Since that promise returned from waitForWifiManagerEventOnce() will not resolve until the specified event is fulfilled, you can't serialize them as usual.  So it will be:

  let promises = [];
  promises.push(waitForWifiManagerEventOnce(aEnabled ? 'enabled' : 'disabled'));
  promises.push(setSettings({ 'wifi.enabled': aEnabled }));
  return Promise.all(promises);

I think I've mentioned that you should have an API for setSettings() and merge the two requestWifi{Enable,Disable} calls.

@@ +281,5 @@
> +      }
> +
> +      // 'interface' is a required field but no need of being configurable
> +      // for a test case. So we initialize this field on our own.
> +      config.interface = 'AP-' + aIndex;

If you ever noticed, I moved this line into startOneHostapd() because it's really a presumption for startOneHostapd() that the config filename should always match the config interface.  Such rearrangement put lines with same purpose in the same function.  It also follows createConfigFromCommon() shouldn't take an index as its argument.  Instead, it should take an regular js object because it's now free from the previous presumption.  As a whole, we have:

  function createConfigFromCommon(aExtraConfig) {
    let config = JSON.parse(JSON.stringify(HOSTAPD_COMMON_CONFIG));
    for (let key in aConfigList[aIndex]) {
      config[key] = aConfigList[aIndex][key];
    }
    return config;
  }

  function startOneHostapd(aConfig, aIndex) {
    let configFileName = HOSTAPD_CONFIG_PATH + 'ap' + aIndex + '.conf';
    aConfig.interface = 'AP-' + aIndex;
    return writeHostapdConfFile(configFileName, aConfig)
      .then(() => runEmulatorShellSafe(['hostapd', '-B', configFileName]));
  }

  return Promise.all(aConfigList.map(function(aExtraConfig, aIndex) {
    return startOneHostapd(createConfigFromCommon(aExtraConfig), aIndex);
  }));

@@ +288,5 @@
> +    }
> +
> +    function startOneHostapd(aIndex) {
> +      let configFileName = HOSTAPD_CONFIG_PATH + 'ap' + aIndex + '.conf';
> +      return writeHostapdConfFile(configFileName, createConfigFromCommon(aIndex)).

nit: don't put a class access period in line end. Move it to the beginning of the line.

@@ +386,5 @@
> +   *
> +   * @return A deferred promise.
> +   */
> +  function isInitServiceRunning(aServiceName) {
> +    return runEmulatorShellSafe(['getprop init.svc.' + aServiceName])

['getprop', 'init.svc.' + aServiceName]

@@ +416,5 @@
> +   */
> +  function startStopInitService(aServiceName, aStart) {
> +    let retryCnt = 10;
> +
> +    return runEmulatorShellSafe([(aStart ? 'start ' : 'stop ') + aServiceName])

I've also mentioned this before.  Please make sure it won't come thrice.

  runEmulatorShellSafe([(aStart ? 'start' : 'stop'), aServiceName])

@@ +429,5 @@
> +        }
> +        if (!controlSuccess) {
> +          return Promise.reject();
> +        }
> +        return; // Resolved!

Bail-out early.

  if (aStart === aIsRunning) {
    return;
  }

  if (retryCnt-- <= 0) {
    // throw turns a resolving promise into a rejected one.
    throw 'hello world';
  }

  // I still feel there should be a time delay between each retry.
  return isInitServiceRunning(aServiceName)
    .then(onIsServiceRunningResolved);

@@ +449,5 @@
> +   * @return A deferred promise.
> +   */
> +  function startStockHostapd() {
> +    return startStopInitService(STOCK_HOSTAPD_NAME, true)
> +      .then(function onresolve() {}, function onreject() {

You can place a null instead.

::: dom/wifi/test/marionette/test_wifi_associate.js
@@ +71,5 @@
> + */
> +function convertToTestAssociatePromises(aNetworks) {
> +  return aNetworks.map(function (aNetwork) {
> +    if (-1 !== getHostapdConfigIndex(aNetwork.ssid)) {
> +      return testAssociate(aNetwork);

I don't know how could this ever work.  The return value here is an array of several __concurrently__ running promises.  How could WiFi manager __concurrently__ process association requests without any error?

@@ +160,5 @@
> +      }
> +      if (!isScanResultExpected) {
> +        return Promise.reject();
> +      }
> +      return networks; // Resolved!

Bail-out early please.

@@ +176,5 @@
> + *
> + * @return A deferred promise.
> + */
> +function testScanResult(aNetworks) {
> +  return aNetworks.length === HOSTAPD_CONFIG_LIST.length;

It's not returning a promise for sure.

@@ +191,5 @@
> +    .then(networks => {
> +      return Promise.all(convertToTestAssociatePromises(networks))
> +        .then(gTestSuite.killAllHostapd)
> +        .then(() => gTestSuite.getProcessDetail('hostapd'))
> +        .then(detail => is(detail.length, 0));

.then(networks => Promise.all(convertToTestAssociatePromises(networks)))
.then(gTestSuite.killAllHostapd)
...

Comment 23

[Henry Chang [:hchang]]

So quick review!

Some reply below:

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> Comment on attachment 8421457 [details] [diff] [review]
> Test case V3
> 
> Review of attachment 8421457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/test/marionette/head.js
> @@ +105,5 @@
> > +        // root      2     0     0      0     c0052c64 00000000 S kthreadd
> > +        // root      3     2     0      0     c0044978 00000000 S ksoftirqd/0
> > +        //
> > +        let detail = [];
> > +        for (let i = 1 /*Skip the first line*/ ; i < processes.length; i++) {
> 
> I don't really like this inline comment.  We can have:
> 

May I know the reason?

>   // Skip the first line.
>   processes.shift();
> 
>   let detail = [];
>   for (let i = 0; i < processes.length; i++) { ... }
> 
> @@ +225,5 @@
> > +      'wifi.enabled': true
> > +    });
> > +
> > +    return wrapDomRequestAsPromise(request)
> > +      .then(() => waitForWifiManagerEventOnce('enabled'));
> 
> Sorry, this can't be done in this way because it may introduce racing
> between event dispatching and listener registration per prior experience. 
> You should always make sure waitForWifiManagerEventOnce() is called
> __before__ you ever make the request.  Since that promise returned from
> waitForWifiManagerEventOnce() will not resolve until the specified event is
> fulfilled, you can't serialize them as usual.  So it will be:
> 
>   let promises = [];
>   promises.push(waitForWifiManagerEventOnce(aEnabled ? 'enabled' :
> 'disabled'));
>   promises.push(setSettings({ 'wifi.enabled': aEnabled }));
>   return Promise.all(promises);
> 
> I think I've mentioned that you should have an API for setSettings() and
> merge the two requestWifi{Enable,Disable} calls.
> 

There is no strong reason to have |setSettings| for now because no other 
test case needs it. Also, I would like to abstract 'enabling/disabling' wifi 
since it is going to have an API like MozWifiManager.enable() MozWifiManager.disable().
Or, you think we shouldn't hide this fact in the test case?

> @@ +281,5 @@
> > +      }
> > +
> > +      // 'interface' is a required field but no need of being configurable
> > +      // for a test case. So we initialize this field on our own.
> > +      config.interface = 'AP-' + aIndex;
> 
> If you ever noticed, I moved this line into startOneHostapd() because it's
> really a presumption for startOneHostapd() that the config filename should
> always match the config interface.  Such rearrangement put lines with same
> purpose in the same function.  It also follows createConfigFromCommon()
> shouldn't take an index as its argument.  Instead, it should take an regular
> js object because it's now free from the previous presumption.  As a whole,

The config file name is not necessary to match the config interface. Appending
the index is just to make each distinct. So, I think they are not for the same purpose.
Do you still think we should rearrange based on my clarification?

> we have:
> 
>   function createConfigFromCommon(aExtraConfig) {
>     let config = JSON.parse(JSON.stringify(HOSTAPD_COMMON_CONFIG));
>     for (let key in aConfigList[aIndex]) {
>       config[key] = aConfigList[aIndex][key];
>     }
>     return config;
>   }
> 
>   function startOneHostapd(aConfig, aIndex) {
>     let configFileName = HOSTAPD_CONFIG_PATH + 'ap' + aIndex + '.conf';
>     aConfig.interface = 'AP-' + aIndex;
>     return writeHostapdConfFile(configFileName, aConfig)
>       .then(() => runEmulatorShellSafe(['hostapd', '-B', configFileName]));
>   }
> 
>   return Promise.all(aConfigList.map(function(aExtraConfig, aIndex) {
>     return startOneHostapd(createConfigFromCommon(aExtraConfig), aIndex);
>   }));
>
> @@ +386,5 @@
> > +   *
> > +   * @return A deferred promise.
> > +   */
> > +  function isInitServiceRunning(aServiceName) {
> > +    return runEmulatorShellSafe(['getprop init.svc.' + aServiceName])
> 
> ['getprop', 'init.svc.' + aServiceName]
> 
> @@ +416,5 @@
> > +   */
> > +  function startStopInitService(aServiceName, aStart) {
> > +    let retryCnt = 10;
> > +
> > +    return runEmulatorShellSafe([(aStart ? 'start ' : 'stop ') + aServiceName])
> 
> I've also mentioned this before.  Please make sure it won't come thrice.

Sorry but what do you refer here? The early bail-out or?

> 
>   runEmulatorShellSafe([(aStart ? 'start' : 'stop'), aServiceName])
> 
> @@ +429,5 @@
> > +        }
> > +        if (!controlSuccess) {
> > +          return Promise.reject();
> > +        }
> > +        return; // Resolved!
> 
> Bail-out early.
> 
>   if (aStart === aIsRunning) {
>     return;
>   }
> 
>   if (retryCnt-- <= 0) {
>     // throw turns a resolving promise into a rejected one.
>     throw 'hello world';

Couldn't it be returning Promise.reject()?

>   }
> 
>   // I still feel there should be a time delay between each retry.

It won't avoid restart stock hostapd twice but it indeed makes more sense...

>   return isInitServiceRunning(aServiceName)
>     .then(onIsServiceRunningResolved);
> 
> ::: dom/wifi/test/marionette/test_wifi_associate.js
> @@ +71,5 @@
> > + */
> > +function convertToTestAssociatePromises(aNetworks) {
> > +  return aNetworks.map(function (aNetwork) {
> > +    if (-1 !== getHostapdConfigIndex(aNetwork.ssid)) {
> > +      return testAssociate(aNetwork);
> 
> I don't know how could this ever work.  The return value here is an array of
> several __concurrently__ running promises.  How could WiFi manager
> __concurrently__ process association requests without any error?
> 

Thanks for pointing out this flaw!

Comment 24

[Henry Chang [:hchang]]

> > @@ +225,5 @@
> > > +      'wifi.enabled': true
> > > +    });
> > > +
> > > +    return wrapDomRequestAsPromise(request)
> > > +      .then(() => waitForWifiManagerEventOnce('enabled'));
> > 
> > Sorry, this can't be done in this way because it may introduce racing
> > between event dispatching and listener registration per prior experience. 
> > You should always make sure waitForWifiManagerEventOnce() is called
> > __before__ you ever make the request.  Since that promise returned from
> > waitForWifiManagerEventOnce() will not resolve until the specified event is
> > fulfilled, you can't serialize them as usual.  So it will be:
> > 
> >   let promises = [];
> >   promises.push(waitForWifiManagerEventOnce(aEnabled ? 'enabled' :
> > 'disabled'));
> >   promises.push(setSettings({ 'wifi.enabled': aEnabled }));
> >   return Promise.all(promises);
> > 
> > I think I've mentioned that you should have an API for setSettings() and
> > merge the two requestWifi{Enable,Disable} calls.
> > 
> 
> There is no strong reason to have |setSettings| for now because no other 
> test case needs it. Also, I would like to abstract 'enabling/disabling' wifi 
> since it is going to have an API like MozWifiManager.enable()
> MozWifiManager.disable().
> Or, you think we shouldn't hide this fact in the test case?
> 

Hmmmmm, since we need a promise-based settings setter to solve the racing issue,
setSettings seems worthy of presence now....

Comment 25

[Henry Chang [:hchang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> (In reply to Henry Chang [:henry] from comment #17)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> > > Review of attachment 8418565 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > 
> > How will they look differently? Could you please explain more or is there
> > existing
> > any example for reference? Thanks!
> 
> suite.doTest = function(aDoTest) {
>   let origEnabled;
>   return addRequiredPermissions()
>     .then(() => {
>       wifiManager = window.navigator.mozWifiManager;
>       origEnabled = wifiManager.enabled;
>     })
>     .then(aDoTest)
>     .then(() => origEnabled ? ensureWifiEnabled() : ensureWifiDisabled())
>     .then(finish);
> };
> 
> suite.doApTest = function(aDoTest) {
>   return suite.doTest(function() {
>     return stopStockHostapd()
>       .then(aDoTest)
>       .then(startStockHostapd);
>   });
> };

I would rather pass an additional flag to conditionally turn off
all running hostapd before the test. What do you think?

Thanks!

Comment 26

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8421457 [details] [diff] [review]
Test case V3

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

::: dom/wifi/test/marionette/test_wifi_associate.js
@@ +71,5 @@
> + */
> +function convertToTestAssociatePromises(aNetworks) {
> +  return aNetworks.map(function (aNetwork) {
> +    if (-1 !== getHostapdConfigIndex(aNetwork.ssid)) {
> +      return testAssociate(aNetwork);

We could try to associate to APs with different security types one by one here. Not sure if we could verify association function when AP's security type is EAP-xxx as well.

@@ +188,5 @@
> +    .then(() => gTestSuite.getProcessDetail('hostapd'))
> +    .then(detail => is(detail.length, HOSTAPD_CONFIG_LIST.length))
> +    .then(() => testScanWithRetry(SCAN_RETRY_CNT))
> +    .then(networks => {
> +      return Promise.all(convertToTestAssociatePromises(networks))

What if scan retry failed ? We should separate scan result to other test case.

Comment 27

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

(In reply to Henry Chang [:henry] from comment #25)
> I would rather pass an additional flag to conditionally turn off
> all running hostapd before the test. What do you think?
> Thanks!

I was trying to sell an Template Method pattern.  You can easily have as many variants as you can by simply passing a new function to doTest().  I'd like to have a extremely simple doTest() that does three things only: 1) permission, 2) invoke test main, and 3) clean ups.  I don't like flags.  Flags mean you have several things to do, several different control paths.  As a reader, that follows you have to remember additional states in order to understand what's really done in that function.  Write trivial code.  You've read of all these principles from tons of books yet you still refuse to simplify a function and still want to add more complexity into a function by introducing a new flag.  What do I think?  I think that sucks.

Comment 28

[Henry Chang [:hchang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #27)
> (In reply to Henry Chang [:henry] from comment #25)
> > I would rather pass an additional flag to conditionally turn off
> > all running hostapd before the test. What do you think?
> > Thanks!
> 
> I was trying to sell an Template Method pattern.  You can easily have as
> many variants as you can by simply passing a new function to doTest().  I'd
> like to have a extremely simple doTest() that does three things only: 1)
> permission, 2) invoke test main, and 3) clean ups.  I don't like flags. 
> Flags mean you have several things to do, several different control paths. 
> As a reader, that follows you have to remember additional states in order to
> understand what's really done in that function.  Write trivial code.  You've
> read of all these principles from tons of books yet you still refuse to
> simplify a function and still want to add more complexity into a function by
> introducing a new flag.  What do I think?  I think that sucks.

Well, so what do you think of |setSettings|? We can also remove the 
|aAllowError| flag by having two functions like |setSettings| and |setSettingsAllowError|.
I do know those principles and patterns but sometimes I would like
to choose the "KISS" principle to keep it stupid simple.
Anyway, I will buy your idea. Thanks!

Comment 29

[Henry Chang [:hchang]]

(In reply to Vincent Chang[:vchang] from comment #26)
> Comment on attachment 8421457 [details] [diff] [review]
> Test case V3
> 
> Review of attachment 8421457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/test/marionette/test_wifi_associate.js
> @@ +71,5 @@
> > + */
> > +function convertToTestAssociatePromises(aNetworks) {
> > +  return aNetworks.map(function (aNetwork) {
> > +    if (-1 !== getHostapdConfigIndex(aNetwork.ssid)) {
> > +      return testAssociate(aNetwork);
> 
> We could try to associate to APs with different security types one by one
> here. Not sure if we could verify association function when AP's security
> type is EAP-xxx as well.
> 

I will rewrite this part to make it serialized but not concurrent.
As for EAP-XXX, I don't even know if hostapd support it :p

> @@ +188,5 @@
> > +    .then(() => gTestSuite.getProcessDetail('hostapd'))
> > +    .then(detail => is(detail.length, HOSTAPD_CONFIG_LIST.length))
> > +    .then(() => testScanWithRetry(SCAN_RETRY_CNT))
> > +    .then(networks => {
> > +      return Promise.all(convertToTestAssociatePromises(networks))
> 
> What if scan retry failed ? We should separate scan result to other test
> case.

The promise will reject if scan fails. I can add new test cases for scan only.
Maybe one for scan when no AP presence, one for all kinds of APs presence.

Thanks!

Comment 30

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

(In reply to Henry Chang [:henry] from comment #28)
> Well, so what do you think of |setSettings|? We can also remove the 
> |aAllowError| flag by having two functions like |setSettings| and
> |setSettingsAllowError|.

What?  |aAllowError| is only referenced once in one line in a function less than 10 lines.  What's your problem?

Comment 31

[Henry Chang [:hchang]]

Attachment: Test case V4

Comment 32

[Henry Chang [:hchang]]

Attachment: Test case V4

Comment 33

[Henry Chang [:hchang]]

Attachment: Test case V4

Comment 34

[Henry Chang [:hchang]]

Major differences from V3 =>

1) Add test_wifi_scan.js for testing scan function with 0 and a couple of APs.
2) Remove the use of "arrow function expression" for non one-line function.
3) Throw exception to reject a promise rather then returning Promise.reject().
4) Add delay to init service running state check.
5) Extract some functions from test_wifi_associate.js to head.js for (1) 
6) Add new test routine |doTestWithoutStockAp| for those tests which requires no AP present before test.
7) Change concurrent WifiManager.associate() to serialized promise chain.
8) Register the event listener before doing any action which may trigger that event being sent.

Comment 35

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

Comment on attachment 8423689 [details] [diff] [review]
Test case V4

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

::: dom/wifi/test/marionette/head.js
@@ +269,5 @@
> +        if (-1 === getFirstIndexBySsid(aScanResult[i].ssid, aExpectedNetworks)) {
> +          return false;
> +        }
> +      }
> +      return true;

If you get empty scan results, it will always return true while you're expecting some valid networks.

@@ +355,5 @@
> +        .then(() => runEmulatorShellSafe(['hostapd', '-B', configFileName]))
> +        .then(function (reply) {
> +          // It may fail at the first time due to the previous ungracefully terminated one.
> +          if (reply[0] === 'bind(PF_UNIX): Address already in use') {
> +            return startOneHostapd(aIndex);

Do we really have to rewrite config file again?

::: dom/wifi/test/marionette/test_wifi_associate.js
@@ +40,5 @@
> +        }
> +
> +        log('Not expected "connected" statuschange event. Wait again!');
> +        return gTestSuite.waitForWifiManagerEventOnce('statuschange')
> +          .then(onstatuschange);

return waitForConnected();

@@ +68,5 @@
> + */
> +function convertToTestAssociateChain(aNetworks) {
> +  let chain = Promise.resolve();
> +
> +  aNetworks.map(function (aNetwork) {

You're not going to produce an array with some mapped values.  Please use Array.forEach() instead.

Comment 36

[Henry Chang [:hchang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #35)
> Comment on attachment 8423689 [details] [diff] [review]
> Test case V4
> 
> @@ +355,5 @@
> > +        .then(() => runEmulatorShellSafe(['hostapd', '-B', configFileName]))
> > +        .then(function (reply) {
> > +          // It may fail at the first time due to the previous ungracefully terminated one.
> > +          if (reply[0] === 'bind(PF_UNIX): Address already in use') {
> > +            return startOneHostapd(aIndex);
> 
> Do we really have to rewrite config file again?

Since hostapd might update the config file, it would better of having a brand new one.

Thanks!

Comment 37

[Henry Chang [:hchang]]

Attachment: Test case V5

Comment 38

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #37)
> Created attachment 8423786 [details] [diff] [review]
> Test case V5

Addressed vicamo's comment @ Comment 35.

Comment 39

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

Comment on attachment 8423786 [details] [diff] [review]
Test case V5

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

I can only give you f+ because some of my previous comments are not going to be addressed.  Nothing obviously wrong.  Thank you.

Comment 40

[Henry Chang [:hchang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #39)
> Comment on attachment 8423786 [details] [diff] [review]
> Test case V5
> 
> Review of attachment 8423786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can only give you f+ because some of my previous comments are not going to
> be addressed.  Nothing obviously wrong.  Thank you.

Thanks :) Do you mean the comments about |startHostapds| ? I will review those again next week. Really appreciate your precious feedbacks on the test cases!

Comment 41

[Henry Chang [:hchang]]

Comment on attachment 8423786 [details] [diff] [review]
Test case V5

Hi Vincent, could you please help review based on vicamo's feedbacks? Thanks!

Comment 42

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8423786 [details] [diff] [review]
Test case V5

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

I think the patch is ready to go, even if it is still not perfect. 
Finally, we have test cases in wifi. 
Thank you, henry. Good jobs.

Comment 43

[Vincent Chang[:vchang][changyihsin]]

Hi Vicamo, thanks for your help to review the patch, it's really helpful. Do you think we can land this patch right now ? I think we can continue to improve the wifi test cases in the follow-up bugs.

Comment 44

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

Comment on attachment 8423786 [details] [diff] [review]
Test case V5

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

::: dom/wifi/test/marionette/head.js
@@ +286,5 @@
> +        if (aRetryCnt > 0) {
> +          return testWifiScanWithRetry(aRetryCnt - 1, aExpectedNetworks);
> +        }
> +        throw 'Unexpected scan result!';
> +      })

nit: missing semi-colon at EOL

@@ +362,5 @@
> +          // It may fail at the first time due to the previous ungracefully terminated one.
> +          if (reply[0] === 'bind(PF_UNIX): Address already in use') {
> +            return startOneHostapd(aIndex);
> +          }
> +        })

ditto.

@@ +550,5 @@
> +    return startStopInitService(STOCK_HOSTAPD_NAME, true)
> +      .then(null, function onreject() {
> +        log('Failed to restart goldfish-hostapd at the first time. Try again!');
> +        return startStopInitService((STOCK_HOSTAPD_NAME), true);
> +      })

ditto.

@@ +701,5 @@
> +        cleanUp();
> +      }, function onreject(aReason) {
> +        ok(false, 'Promise rejects during test' + (aReason ? '(' + aReason + ')' : ''));
> +        cleanUp();
> +      })

ditto.

::: dom/wifi/test/marionette/test_wifi_scan.js
@@ +38,5 @@
> +
> +gTestSuite.doTestWithoutStockAp(function() {
> +  return gTestSuite.ensureWifiEnabled(true)
> +    .then(testScanNoAp)
> +    .then(testScanWithAps)

ditto.

Comment 45

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

(In reply to Vincent Chang[:vchang] from comment #43)
> Hi Vicamo, thanks for your help to review the patch, it's really helpful. Do
> you think we can land this patch right now ? I think we can continue to
> improve the wifi test cases in the follow-up bugs.

I've give my f+ in comment 39.  Just found some semi-colons are missed.  Nothing special.

Comment 46

[Henry Chang [:hchang]]

Attachment: Test case V6 (r+'d)

Waiting for test result: https://tbpl.mozilla.org/?tree=Try&rev=d77fa00c34fe

Comment 47

[Ryan VanderMeulen [:RyanVM]]

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

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df9895aa0ca0