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)
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 |
See comment bug 1180589 comment #26
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
(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)
Assignee | ||
Comment 4•9 years ago
|
||
This patch is relied on bug 1125477. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b439fbeefd7b
Assignee | ||
Comment 5•9 years ago
|
||
Previous patch failed build. https://treeherder.mozilla.org/#/jobs?repo=try&revision=318b9f602e0e
Attachment #8679829 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
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
Assignee | ||
Comment 7•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → mantaroh
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Applied the change of bug 1125477. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d2f947e1782
Attachment #8682968 -
Attachment is obsolete: true
Attachment #8684050 -
Flags: review?(selin)
Comment 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
Sorry, I have submitted invalid patch. I reattached correct patch.
Attachment #8684050 -
Attachment is obsolete: true
Attachment #8684721 -
Flags: review?(selin)
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
Thanks Sean! I modified some code of comment #15. Could you confirm again?
Attachment #8686461 -
Attachment is obsolete: true
Attachment #8687049 -
Flags: review?(selin)
Updated•9 years ago
|
Attachment #8687049 -
Flags: review?(selin) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f127d642f07
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 18•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c9cfbe159b26
Keywords: checkin-needed
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/5a6b4eae6d8e2d67b0c50fe84ed30b4794bf25ea Backed out changeset c9cfbe159b26 (bug 1200133) for failing it's own tests
Comment 21•9 years ago
|
||
Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3427460&repo=b2g-inbound
Assignee | ||
Comment 22•9 years ago
|
||
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
Assignee | ||
Comment 23•8 years ago
|
||
Assignee | ||
Comment 24•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91c5a1b05b6e&group_state=expanded
Assignee | ||
Comment 25•8 years ago
|
||
Rebased and change commit message. carrying forward r+ from obsolete
Attachment #8691252 -
Attachment is obsolete: true
Attachment #8693439 -
Flags: review+
Assignee | ||
Comment 26•8 years ago
|
||
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 27•8 years ago
|
||
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+
Assignee | ||
Comment 28•8 years ago
|
||
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 29•8 years ago
|
||
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+
Assignee | ||
Comment 30•8 years ago
|
||
Thanks Sean! I reabsed it.
Attachment #8694537 -
Attachment is obsolete: true
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8699685 [details] [diff] [review] Change mochitest using TVSimulatorService. Carrying forward 'r+' from seanlin.
Attachment #8699685 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 32•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&author=mantaroh@gmail.com
Comment 33•8 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/ffa8ce8f2aa7
Keywords: checkin-needed
Comment 34•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ffa8ce8f2aa7
You need to log in
before you can comment on or make changes to this bug.
Description
•