If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[Stingray] Add more tests to TV Manager API

RESOLVED FIXED in 2.1 S8 (7Nov)

Status

Firefox OS
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seanlin, Assigned: seanlin)

Tracking

unspecified
2.1 S8 (7Nov)
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FT:Stream3])

Attachments

(1 attachment, 4 obsolete attachments)

We've implemented the primary logic of TV Manager API in bug 998872. Yet we still need to add some more test cases for it.
Depends on: 998872
Created attachment 8510622 [details] [diff] [review]
Patch v1

I couldn't use the way in DataStore tests to install hosted apps since so far I haven't found a way to let SpecialPowers apply 'tv' permission to the new iframe of the hosted app. Instead I simply enable |dom.testing.tv_enabled_for_hosted_apps| and test against the current window as a workaround.
Assignee: nobody → selin
Attachment #8510622 - Flags: review?(ehsan.akhgari)
Comment on attachment 8510622 [details] [diff] [review]
Patch v1

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

Please submit the next interdiff with diff -w so that the diff doesn't contain whitespace changes as a result of fixing indentations, etc.  Thanks!

::: dom/tv/FakeTVService.cpp
@@ +110,5 @@
> +    mTuners[i]->GetId(tunerId);
> +    if (aTunerId.Equals(tunerId)) {
> +      uint32_t sourceTypeCount;
> +      char** sourceTypes;
> +      mTuners[0]->GetSupportedSourceTypes(&sourceTypeCount, &sourceTypes);

Please make sure that you deallocate the array when you're done with it.

@@ +111,5 @@
> +    if (aTunerId.Equals(tunerId)) {
> +      uint32_t sourceTypeCount;
> +      char** sourceTypes;
> +      mTuners[0]->GetSupportedSourceTypes(&sourceTypeCount, &sourceTypes);
> +      for (uint32_t j = 0; j < sourceTypeCount; i++) {

j++?

@@ +150,5 @@
> +    mSourceListener->NotifyChannelScanComplete(mTunerId, mSourceType);
> +    return NS_OK;
> +  }
> +
> +  nsString mTunerId;

Nit: private.

@@ +177,5 @@
> +  nsString tunerId;
> +  mTuners[0]->GetId(tunerId);
> +  uint32_t sourceTypeCount;
> +  char** sourceTypes;
> +  mTuners[0]->GetSupportedSourceTypes(&sourceTypeCount, &sourceTypes);

Here too.

@@ +202,5 @@
> +
> +    // Set a timer. |notifyChannelScanComplete| will be called after the timer
> +    // fires (10s). (The timer could be canceled if |StopScanningChannels| gets
> +    // called before firing.)
> +    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);

I think do_CreateInstance can return null so you should do a null check before dereferencing mTimer.

@@ +203,5 @@
> +    // Set a timer. |notifyChannelScanComplete| will be called after the timer
> +    // fires (10s). (The timer could be canceled if |StopScanningChannels| gets
> +    // called before firing.)
> +    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    ScanCompleteCallback* cb =

Please use nsRefPtr<ScanCompleteCallback> to avoid a leak when InitWithCallback fails.

@@ +220,5 @@
>    if (!aCallback) {
>      return NS_ERROR_INVALID_ARG;
>    }
>  
> +  mTimer->Cancel();

You should probably null check here too.

@@ +230,5 @@
>  }
>  
>  /* virtual */ NS_IMETHODIMP
>  FakeTVService::ClearScannedChannelsCache()
>  {

Maybe add a comment saying we don't need to do anything here because we don't have a channel cache.

@@ +254,5 @@
> +  nsString tunerId;
> +  mTuners[0]->GetId(tunerId);
> +  uint32_t sourceTypeCount;
> +  char** sourceTypes;
> +  mTuners[0]->GetSupportedSourceTypes(&sourceTypeCount, &sourceTypes);

Ditto.

@@ +299,5 @@
> +  nsString tunerId;
> +  mTuners[0]->GetId(tunerId);
> +  uint32_t sourceTypeCount;
> +  char** sourceTypes;
> +  mTuners[0]->GetSupportedSourceTypes(&sourceTypeCount, &sourceTypes);

And here.

@@ +336,5 @@
> +  nsString tunerId;
> +  mTuners[0]->GetId(tunerId);
> +  uint32_t sourceTypeCount;
> +  char** sourceTypes;
> +  mTuners[0]->GetSupportedSourceTypes(&sourceTypeCount, &sourceTypes);

And here.

::: dom/tv/FakeTVService.h
@@ +64,4 @@
>    nsCOMPtr<nsITVSourceListener> mSourceListener;
> +  nsTArray<nsCOMPtr<nsITVTunerData>> mTuners;
> +  nsTArray<nsCOMPtr<nsITVChannelData>> mChannels;
> +  nsTArray<nsCOMPtr<nsITVProgramData>> mPrograms;

Please add a comment here saying that a real implementation may want to use more efficient data structures, in case someone tries to model their implementation on top of this code!

::: dom/tv/test/test_tv_channel_data.html
@@ +12,5 @@
> +<script type="application/javascript">
> +
> +"use strict";
> +
> +SimpleTest.waitForExplicitFinish();

This and SimpleTest.finish() are only useful for asynchronous tests.  For synchronous tests, you don't need them, and the test will finish when the page's load event gets dispatched.

Actually, since this test only tests one XPCOM object, you should convert it to an xpcshell test.

Same for other similar tests.

::: dom/tv/test/test_tv_get_channels.html
@@ +14,5 @@
> +"use strict";
> +SimpleTest.waitForExplicitFinish();
> +
> +SpecialPowers.pushPrefEnv({"set": [["dom.tv.enabled", true],
> +                                   ["dom.testing.tv_enabled_for_hosted_apps", true]]}, function() {

As discussed in person, we should try to push the "tv" permission and load the iframe after.

@@ +17,5 @@
> +SpecialPowers.pushPrefEnv({"set": [["dom.tv.enabled", true],
> +                                   ["dom.testing.tv_enabled_for_hosted_apps", true]]}, function() {
> +  SpecialPowers.addPermission("tv", true, document);
> +
> +  ok('tv' in navigator, "navigator.tv should exist.");

And the rest of this needs to move to the iframe...

@@ +21,5 @@
> +  ok('tv' in navigator, "navigator.tv should exist.");
> +
> +  var tv = navigator.tv;
> +  tv.getTuners().then(
> +    function(aTuners){

Nit: space before curly braces.

@@ +40,5 @@
> +                ok(channel.transportStreamId, "Channel " + i + " should have a transport stream ID.");
> +                ok(channel.serviceId, "Channel " + i + " should have a service ID.");
> +                ok(channel.type, "Channel " + i + " should have a type.");
> +                ok(channel.name, "Channel " + i + " should have a name.");
> +                ok(channel.number, "Channel " + i + " should have a number.");

I think these checks may be wrong.  For example, this test is going to fail if the channel number is 0.  

If you want to test the existence of the attribute, you can do: "number" in channel.  Or if you want to test whether channel is a TVChannel, you can do an instanceof check.

Please make this change everywhere in these tests.

::: dom/tv/test/test_tv_get_programs.html
@@ +37,5 @@
> +                function(aPrograms){
> +                  ok(aPrograms.length > 0, "Got at least 1 program.");
> +
> +                  for(var i = 0; i < aPrograms.length; i++){
> +                	var program = aPrograms[i];

Nit: please don't use tabs in indentations.

::: dom/tv/test/test_tv_program_data.html
@@ +12,5 @@
> +<script type="application/javascript">
> +
> +"use strict";
> +
> +SimpleTest.waitForExplicitFinish();

See the comment before.

::: dom/tv/test/test_tv_scan_channels_completed.html
@@ +36,5 @@
> +
> +          source.oneitbroadcasted = function(aEvent){
> +            isEITEventFired = true;
> +
> +        	var programs = aEvent.programs;

Nit: the indentation is all off here.

@@ +52,5 @@
> +        	}
> +          };
> +
> +          source.onscanningstatechanged = function(aEvent){
> +            ok(aEvent.state, "Received scanning event with state: " +

You should test to make sure the event state is what you expect it to be.

@@ +69,5 @@
> +              ok(channel.number, "Channel should have a number.");
> +            }
> +          };
> +
> +          source.startScanning({}).then(

As discussed in person, we need to resolve the promise returned from startScanning after the entire scanning operation has been finished.  This obviously requires spec changes as well.  Please file a follow-up bug about this, and then add a comment here pointing to that bug number saying that this timer magic can be removed once that bug gets fixed.

::: dom/tv/test/test_tv_scan_channels_stopped.html
@@ +35,5 @@
> +          var source = aSources[0];
> +
> +          source.onscanningstatechanged = function(aEvent){
> +            ok(aEvent.state, "Received scanning event with state: " +
> +                              aEvent.state + ".");

Here too.

@@ +44,5 @@
> +              isStoppedEventFired = true;
> +            }
> +          };
> +
> +          source.startScanning({ isRescanned: true }).then(

Here too.

@@ +46,5 @@
> +          };
> +
> +          source.startScanning({ isRescanned: true }).then(
> +            function(){
> +              source.stopScanning().then(

We should test that this call rejects the promise returned from startScanning later too.

::: dom/tv/test/test_tv_set_current_channel.html
@@ +49,5 @@
> +              };
> +
> +              source.setCurrentChannel(selectedChannelNumber).then(
> +                function(){
> +                  isOperationComplete = true;

Please make sure that getCurrentChannel would return the channel that you'd expect here.

@@ +76,5 @@
> +    }
> +  );
> +
> +  var timer = setInterval(function() {
> +    if (isEventFired && isOperationComplete) {

This pattern of testing these flags when deciding to terminate the test will make sure that all of the intended events have happened, but we also need to test the order in which they happened.  For example, we need to make sure that isEventFired is true by the time that the promise returned from setCurrentChannel is resolved.  Same thing in all of the other tests with this pattern.

::: dom/tv/test/test_tv_set_invalid_current_channel.html
@@ +36,5 @@
> +              ok(false, "Setting an invalid current channel should get error.");
> +              SimpleTest.finish();
> +            },
> +            function(aError){
> +              ok(true, "Error expected: " + aError);

Please test the kind of error that you're expecting here and elsewhere.
Attachment #8510622 - Flags: review?(ehsan.akhgari) → review-
Comment hidden (obsolete)
Created attachment 8511915 [details] [diff] [review]
Patch diff v1 v2

Testing against the installed hosted app and updating based on latest comments.
Attachment #8510622 - Attachment is obsolete: true
Attachment #8511915 - Flags: review?(ehsan.akhgari)
Comment on attachment 8511915 [details] [diff] [review]
Patch diff v1 v2

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

What happened to those xpcshell tests?  They are missing from the patch.

::: dom/tv/FakeTVService.cpp
@@ +153,5 @@
>      mTuners[i]->GetId(tunerId);
>      if (aTunerId.Equals(tunerId)) {
>        uint32_t sourceTypeCount;
>        char** sourceTypes;
>        mTuners[0]->GetSupportedSourceTypes(&sourceTypeCount, &sourceTypes);

You're still leaking memory here.

@@ +219,3 @@
>    nsString mTunerId;
>    nsString mSourceType;
>    nsRefPtr<nsITVSourceListener> mSourceListener;

Nit: for consistency, use nsCOMPtr here as well.

@@ +248,5 @@
>      mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    NS_ENSURE_TRUE(mTimer, NS_ERROR_OUT_OF_MEMORY);
> +    nsRefPtr<ScanCompleteCallback> cb =
> +      new ScanCompleteCallback(aTunerId, aSourceType, mSourceListener, mChannels[0]);
> +    rv = mTimer->InitWithCallback(cb, 10000, nsITimer::TYPE_ONE_SHOT);

Waiting 10 seconds here is probably excessive.  How about we wait a much shorter amount of time, let's say 10ms?

@@ +269,2 @@
>    mTimer->Cancel();
> +  }

You probably want to set mTimer to nullptr after canceling it.

@@ +397,5 @@
>    return NS_DispatchToCurrentThread(runnable);
>  }
>  
> +bool
> +FakeTVService::isAllowed(const nsAString& aTunerId,

Nit: call this IsAllowed, please.

::: dom/tv/FakeTVService.h
@@ +39,4 @@
>  
>    void Init();
>  
> +  void Shutdown();

These should all be private, I think.

::: dom/tv/test/file_tv_get_channels.html
@@ +13,5 @@
> +  }
> +
> +  function finish() {
> +    alert('DONE');
> +  }

These helper functions can go inside a testhelpers.js file that is shared with all of these file_*.html's.

::: dom/tv/test/file_tv_scan_channels_completed.html
@@ +45,5 @@
> +          };
> +
> +          source.onscanningstatechanged = function(aEvent) {
> +            if (aEvent.state === 'scanned') {
> +              ok(true, "Received channel scanned event.");

Nit: please use info() for these types of log messages instead of ok(true).

::: dom/tv/test/test_tv_get_channels.html
@@ +15,5 @@
> +
> +var gAppsService = SpecialPowers.Cc["@mozilla.org/AppsService;1"]
> +                   .getService(SpecialPowers.Ci.nsIAppsService);
> +
> +var gHostedManifestURL = window.location.origin + '/tests/dom/tv/test/file_app.sjs?testToken=file_tv_get_channels.html&template=file_app.template.webapp';

Is your patch missing file_app.sjs and file_app.template.webapp?

@@ +94,5 @@
> +  }
> +
> +  var test = tests.shift();
> +  test();
> +}

I would add a file named head.js and put everything above except for the tests array in that file.  You can create a function like setupTest which takes the file names above as arguments and just call that with different arguments in each of these test files.  That way, we'd avoid having N copies of this code.

@@ +116,1 @@
>  });

I'd move this bit to the beginning of runTest in head.js too.
Attachment #8511915 - Flags: review?(ehsan.akhgari) → review-
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #5)
> Comment on attachment 8511915 [details] [diff] [review]
> Patch diff v1 v2
> 
> Review of attachment 8511915 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/tv/test/test_tv_get_channels.html
> @@ +15,5 @@
> > +
> > +var gAppsService = SpecialPowers.Cc["@mozilla.org/AppsService;1"]
> > +                   .getService(SpecialPowers.Ci.nsIAppsService);
> > +
> > +var gHostedManifestURL = window.location.origin + '/tests/dom/tv/test/file_app.sjs?testToken=file_tv_get_channels.html&template=file_app.template.webapp';
> 
> Is your patch missing file_app.sjs and file_app.template.webapp?

They've been existing since patch part 1 for bug 998872. And they were not displayed since there was no change for them in this diff.
Created attachment 8512511 [details] [diff] [review]
Patch diff v2 v3

Updating based on the latest comments.
Attachment #8511915 - Attachment is obsolete: true
Attachment #8512511 - Flags: review?(ehsan.akhgari)
Comment on attachment 8512511 [details] [diff] [review]
Patch diff v2 v3

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

::: dom/tv/FakeTVService.cpp
@@ +250,5 @@
>      mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
>      NS_ENSURE_TRUE(mTimer, NS_ERROR_OUT_OF_MEMORY);
>      nsRefPtr<ScanCompleteCallback> cb =
>        new ScanCompleteCallback(aTunerId, aSourceType, mSourceListener, mChannels[0]);
> +    rv = mTimer->InitWithCallback(cb, 100, nsITimer::TYPE_ONE_SHOT);

How about 10? :)

::: dom/tv/test/mochitest/head.js
@@ +22,5 @@
> +
> +    var appId = gAppsService.getAppLocalIdByManifestURL(gApp.manifestURL);
> +    SpecialPowers.addPermission("tv", true, { url: gApp.origin,
> +                                          appId: appId,
> +                                          isInBrowserElement: false });

Nit: please fix the indentation.
Attachment #8512511 - Flags: review?(ehsan.akhgari) → review+
Created attachment 8513339 [details] [diff] [review]
Patch v3

Updating with the latest r+ comments.
Attachment #8512511 - Attachment is obsolete: true
Attachment #8513339 - Flags: review+
The latest try-run. FYI.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0557785c9360
Keywords: checkin-needed
Whiteboard: [FT:Stream3]
Created attachment 8514071 [details] [diff] [review]
Patch v3
Attachment #8513339 - Attachment is obsolete: true
Attachment #8514071 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/09507e031051
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/09507e031051
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
You need to log in before you can comment on or make changes to this bug.