Closed Bug 1200133 Opened 9 years ago Closed 8 years ago

[Simulator] mochitest of TV Manager API should change for TV Simulator Service.

Categories

(Firefox OS Graveyard :: Simulator, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Tracking Status
firefox46 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(1 file, 13 obsolete files)

43.72 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
As bug 1180589 comment #18, the current TV API mochitests should switch to use the simulator service and make it covered by those tests.
Depends on: 1125477
Depends on: 1214520
Hi sean,

I have a question about current mochitests.

It seems that current mochitest skipped B2G and e10s. Is it right?
So Could you please tell me the environment which can test dom/tv/test/mochitest ?

|TVSimulatorService| will refer to Gaia's profile directory, So I think we can't test except for b2g now.
Flags: needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> Hi sean,
> 
> I have a question about current mochitests.
> 
> It seems that current mochitest skipped B2G and e10s. Is it right?

Yeah, due to bug 1125477.

> So Could you please tell me the environment which can test
> dom/tv/test/mochitest ?

They can be run on the browser build.

> 
> |TVSimulatorService| will refer to Gaia's profile directory, So I think we
> can't test except for b2g now.

Then it looks we need to refactor TV mochitests for bug 1125477 first. And then we may also possibly need to run them on b2g only, or tweak the simulator service to load data from somewhere other than Gaia profile for non-b2g platforms.
Flags: needinfo?(selin)
The test result of previous patch is failed. It seems that |TVServiceFactory| can't create the TVSimulatorService.
This cause is that I forgot the the define to the package-manifest.in.

So I added the TVSimulatorService stuff to the browser/installer/package-manifest.in.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acbda15f9d47
Attachment #8679840 - Attachment is obsolete: true
Comment on attachment 8681006 [details] [diff] [review]
Change mochitest using TVSimulatorService.

Hi Sean,

I use TVSimulatorService instead of FakeTVService in mochitest.
So We can same test on simulator.

Could you review it?
Attachment #8681006 - Flags: review?(selin)
Assignee: nobody → mantaroh
Comment on attachment 8681006 [details] [diff] [review]
Change mochitest using TVSimulatorService.

Remove 'WIP' keyword from attach description.
Attachment #8681006 - Attachment description: WIP : Change mochitest using TVSimulatorService. → Change mochitest using TVSimulatorService.
sorry.
Previous patch failed B2G Emulator / Fennec Emulator. The fail cause is that we can't refer the test dummy data.
So I fixed position of dummy data define.
Attachment #8681006 - Attachment is obsolete: true
Attachment #8681006 - Flags: review?(selin)
Comment on attachment 8684050 [details] [diff] [review]
Change mochitest using TVSimulatorService.

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

This patch doesn't seem relevant to the TV service. Is there something missing?
Attachment #8684050 - Flags: review?(selin)
Sorry, I have submitted invalid patch.
I reattached correct patch.
Attachment #8684050 - Attachment is obsolete: true
Attachment #8684721 - Flags: review?(selin)
Comment on attachment 8684721 [details] [diff] [review]
Change mochitest using TVSimulatorService.

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

::: dom/tv/TVServiceFactory.cpp
@@ +21,3 @@
>    nsCOMPtr<nsITVService> service = do_CreateInstance(TV_SERVICE_CONTRACTID);
>    if (!service) {
> +    if (Preferences::GetBool("dom.testing.tv_is_testing", false)) {

The preference check doesn't appear necessarily. We may simply fall back to the simulator service once there's no regular service, just like what we did in [1] for the fake service.

[1] https://hg.mozilla.org/mozilla-central/raw-annotate/544aa604483f/dom/tv/TVServiceFactory.cpp

::: dom/tv/TVSimulatorService.js
@@ +17,4 @@
>  
>  const TV_SIMULATOR_DUMMY_DIRECTORY = "dummy";
>  const TV_SIMULATOR_DUMMY_FILE      = "settings.json";
> +const TV_SIMULATOR_TEST_DUMMY_PREF = "dom.testing.tv_is_testing";

The constant could be removed since it's not used anywhere in the file.

@@ +79,5 @@
>        debug("Error occurred : " + e );
> +
> +      let isTesting = Services.prefs.getBoolPref("dom.testing.tv_is_testing");
> +      if (isTesting) {
> +        settingStr = JSON.stringify(TV_DUMMY_DATA);

Instead of trying to load the testing mock once the service fails to load the simulator settings, I'm more inclined to load different settings based on the preference check. (nit: how about rename the preference as "dom.testing.tv_mock_enabled"?) This would prevent the testing mock data, which are supposed for mochitest only, from being accidentally exposed to the TV simulator if settings loading unfortunately fails.

In |_getFilePath| [1], something like the following could be used:

if (Services.prefs.getBoolPref("dom.testing.tv_is_testing")) {
  // Get the path of the local file for testing mock
} else {
  // The current logic to get the file from dummy directory
}

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVSimulatorService.js#448

@@ +544,5 @@
>  
>  this.NSGetFactory = XPCOMUtils.generateNSGetFactory([TVSimulatorService]);
> +
> +// TV Dummy Data for testing 
> +const TV_DUMMY_DATA = {

It'd better to move the mock data to a local file. Then the simulator service could load the test settings from a file, just like what it does for simulation.

::: dom/tv/test/mochitest/head.js
@@ +3,5 @@
> +var testEnv = {"set": [
> +                ["dom.tv.enabled", true],
> +                ["dom.testing.tv_is_testing", true],
> +                ["dom.ignore_webidl_scope_checks", true]
> +              ]};

This variable is only used once, so it looks more clear to me to simply put this part inline at |pushPrefEnv|.
Attachment #8684721 - Flags: review?(selin)
Thanks Sean,

(In reply to Sean Lin [:seanlin] from comment #13)
> Instead of trying to load the testing mock once the service fails to load
> the simulator settings, I'm more inclined to load different settings based
> on the preference check. (nit: how about rename the preference as
> "dom.testing.tv_mock_enabled"?) This would prevent the testing mock data,
> which are supposed for mochitest only, from being accidentally exposed to
> the TV simulator if settings loading unfortunately fails.
I modified logic of loading TV data.
First, We will check the preferences|dom.testing.tv_mock_enabled|. And if preference is true, we will load from dummy data of mochitest. 

> It'd better to move the mock data to a local file. Then the simulator
> service could load the test settings from a file, just like what it does for
> simulation.
I would like to separate the test data and product logic parts as much as possible. 
However, Mochitest is running on the server of host machine. So we can't access the test mock file as local file.
(I'm thinking of adding the test mock data to mochitest tree, not dom/tv tree.)

e.g. 
 In case of Fennec, We access to the mochitest server on host machine.
 However TVSimulatorService is running on the device. So TVSimulatorService can't access the test mock data.

So I added logic of loading using XMLHttpRequest.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58490a741b2a
Attachment #8684721 - Attachment is obsolete: true
Attachment #8686461 - Flags: review?(selin)
Comment on attachment 8686461 [details] [diff] [review]
Change mochitest using TVSimulatorService.

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

Great. r=me once the following comments are addressed.

::: dom/tv/TVSimulatorService.js
@@ +17,4 @@
>  
> +const TV_SIMULATOR_DUMMY_DIRECTORY   = "dummy";
> +const TV_SIMULATOR_DUMMY_FILE        = "settings.json";
> +const TV_SIMULATOR_ENABLE_MOCK_PREF = "dom.testing.tv_mock_enabled";

nit: since we have |dom.testing.tv_mock_path|, |dom.testing.tv_mock_enabled| doesn't seem necessarily.

@@ +17,5 @@
>  
> +const TV_SIMULATOR_DUMMY_DIRECTORY   = "dummy";
> +const TV_SIMULATOR_DUMMY_FILE        = "settings.json";
> +const TV_SIMULATOR_ENABLE_MOCK_PREF = "dom.testing.tv_mock_enabled";
> +const TV_SIMULATOR_MOCK_DATA_PATH    = "dom.testing.tv_mock_path";

nit: |const TV_SIMULATOR_MOCK_DATA_PATH = Services.prefs.getCharPref("dom.testing.tv_mock_path");| looks better. And please watch the alignment of '='.

@@ +56,3 @@
>      try {
> +      let isTesting = Services.prefs.getBoolPref(TV_SIMULATOR_ENABLE_MOCK_PREF);
> +      if (isTesting) {

nit: |isTesting| doesn't appear necessarily. Please use |if (TV_SIMULATOR_MOCK_DATA_PATH)| instead.

@@ +429,5 @@
> +                    .createInstance(Ci.nsIFileInputStream);
> +    let cstream = Cc["@mozilla.org/intl/converter-input-stream;1"]
> +                   .createInstance(Ci.nsIConverterInputStream);
> +
> +    let retStr = "";

nit: the original name |settingsStr| looks more self-explanatory.

@@ +454,5 @@
> +    return retStr;
> +  },
> +
> +  _getTestMockData : function TVSiimGetMockData() {
> +    let testPath = Services.prefs.getCharPref(TV_SIMULATOR_MOCK_DATA_PATH);

nit: no need since we've loaded it to |TV_SIMULATOR_MOCK_DATA_PATH| constant.

@@ +456,5 @@
> +
> +  _getTestMockData : function TVSiimGetMockData() {
> +    let testPath = Services.prefs.getCharPref(TV_SIMULATOR_MOCK_DATA_PATH);
> +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance();
> +    let retStr;

nit: no need.

@@ +458,5 @@
> +    let testPath = Services.prefs.getCharPref(TV_SIMULATOR_MOCK_DATA_PATH);
> +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance();
> +    let retStr;
> +
> +    xhr.open("GET", testPath, false);

nit: |xhr.open("GET", TV_SIMULATOR_MOCK_DATA_PATH, false);|

@@ +462,5 @@
> +    xhr.open("GET", testPath, false);
> +    xhr.send(null);
> +    if (xhr.status == 200) {
> +      debug("success response!{" + xhr.responseText + "]\n");
> +      retStr = xhr.responseText;

nit: |return xhr.responseText;|

@@ +466,5 @@
> +      retStr = xhr.responseText;
> +    } else {
> +      debug("failed to request xmlhttp["+ xhr.status + "]\n");
> +    }
> +    return retStr;

nit: |return "";|

::: dom/tv/test/mochitest/head.js
@@ +12,5 @@
> +  SpecialPowers.pushPrefEnv({"set": [
> +                              ["dom.tv.enabled", true],
> +                              ["dom.testing.tv_mock_enabled", true],
> +                              ["dom.ignore_webidl_scope_checks", true],
> +                              ["dom.testing.tv_mock_path", window.location.origin + "/tests/dom/tv/test/mochitest/mock/mock_data.json"]

nit: |SimpleTest.getTestFileURL("mock_data.json")| could be used to get the URL.

nit: since we have |dom.testing.tv_mock_path|, |dom.testing.tv_mock_enabled| doesn't seem necessarily.

@@ +18,3 @@
>      callback();
>    });
> +console.log("location[" + window.location + "]\n");

nit: please remove this debug line.

::: dom/tv/test/mochitest/mochitest.ini
@@ +1,4 @@
>  [DEFAULT]
> +support-files =
> +  head.js
> +  mock/mock_data.json

nit: you may simply put |mock_data.json| at the same level rather than a subfolder.
Attachment #8686461 - Flags: review?(selin) → review+
Thanks Sean!

I modified some code of comment #15.
Could you confirm again?
Attachment #8686461 - Attachment is obsolete: true
Attachment #8687049 - Flags: review?(selin)
Attachment #8687049 - Flags: review?(selin) → review+
Keywords: checkin-needed
Keywords: checkin-needed
rebased.
Carrying forward r+.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=416fb7431102
(note:The exception of B2G Emulator's test related bug 1227180.)
Attachment #8687049 - Attachment is obsolete: true
Attachment #8691252 - Flags: review+
Keywords: checkin-needed
Oh, sorry,,

It seems problem of B2G emulator(ICS Emulator) only.
Maybe the cause of this failure related assertion of nsXMLHttpRequest.
I will continue to investigate..

[1] https://dxr.mozilla.org/mozilla-central/rev/35916735b8afc5b0732e00f9aeb56bf846bba7f4/dom/base/nsXMLHttpRequest.cpp#2928
Attached patch bug1200133.patch (obsolete) — Splinter Review
Rebased and change commit message.
carrying forward r+ from obsolete
Attachment #8691252 - Attachment is obsolete: true
Attachment #8693439 - Flags: review+
Hi Sean,

According to B2G emulator's mochitest result, occurred Assertion in |nsXMLHttpRequest::Send()|[1]. This cause is returned NS_ERROR_ILLEGAL_VALUE in |HttpChannelChild::ContinueAsyncOpen()|[2]. Perhaps the problem of this bug is failed to get TabChild in ContinueAsyncOpen().

In this mochitest, I think we can move the loading logic to the mochitest parts(especially, head.js). So I modified loading logic code.

Could you confirm this patch?


[1] https://dxr.mozilla.org/mozilla-central/rev/7883e81f3c305078353ca27a6b1adb8c769d5904/dom/base/nsXMLHttpRequest.cpp#2928
[2] https://dxr.mozilla.org/mozilla-central/rev/7883e81f3c305078353ca27a6b1adb8c769d5904/netwerk/protocol/http/HttpChannelChild.cpp#1796
Attachment #8693405 - Attachment is obsolete: true
Attachment #8693446 - Flags: review?(selin)
Comment on attachment 8693446 [details] [diff] [review]
Part2. Move code of loading settings to mochitest's code.

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

r=me after squashing these changes into one patch. Thank you.
Attachment #8693446 - Flags: review?(selin) → review+
Thanks Sean.

> r=me after squashing these changes into one patch. Thank you.
OK.I merged patch.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=922f3deb8c07
Attachment #8693439 - Attachment is obsolete: true
Attachment #8693446 - Attachment is obsolete: true
Attachment #8694537 - Flags: review?(selin)
Comment on attachment 8694537 [details] [diff] [review]
Change mochitest using TVSimulatorService.

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

Sorry for late response. It looks good to me.;)
Attachment #8694537 - Flags: review?(selin) → review+
Thanks Sean!

I reabsed it.
Attachment #8694537 - Attachment is obsolete: true
Comment on attachment 8699685 [details] [diff] [review]
Change mochitest using TVSimulatorService.

Carrying forward 'r+' from seanlin.
Attachment #8699685 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ffa8ce8f2aa7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: