1190329 - Test coverage for webNavigation extension API

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Gabor Krizsanits (INACTIVE)]

https://wiki.mozilla.org/Browser_Extensions#List_of_supported_APIs

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This just copies the test we have for the .jsm.

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8689834 [details] [diff] [review]
patch

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

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html
@@ +55,5 @@
> +    browser.test.assertTrue(details.frameId !== undefined);
> +    browser.test.assertTrue(details.parentFrameId !== undefined);
> +  }
> +
> +  var listeners = {};

This doesn't appear to actually be used.

@@ +86,5 @@
> +  received = [];
> +  waitingEvent = event;
> +  waitingURL = url;
> +  dump(`RUN ${script}\n`);
> +  eval(script);

Can you make this a lambda rather than using eval?

Comment 3

[Andy McKay]

Luca, it looks like there's a patch from billm that we could probably clean up and merge?

Also looking at code coverage: https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-webNavigation.js.html there's a few small error conditions that need testing, should we make a seperate bug for those?

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Andy McKay [:andym] from comment #3)
> Luca, it looks like there's a patch from billm that we could probably clean
> up and merge?

That patch has already landed in the following changeset (and then tweaked during the implementation of the other webNavigation features):

- https://hg.mozilla.org/mozilla-central/rev/5af1998a1366
 
> Also looking at code coverage:
> https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/
> components/extensions/ext-webNavigation.js.html there's a few small error
> conditions that need testing, should we make a seperate bug for those?

I see, I'm going to look into the scenarios to which they correspond (and a brief look on the other changes that Bug 1190687 is going to introduce into the same file) and then file bugs with descriptions of the scenarios that are not yet covered.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8689834 [details] [diff] [review]
patch

Marking as obsolete this patch because it is already landed as:

- https://hg.mozilla.org/mozilla-central/rev/5af1998a1366

Comment 6

[Andy McKay]

So close, 97.26% https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-webNavigation.js.html

Comment 7

[Andy McKay]

Currently at 99.16% https://coveralls.io/builds/12804880/source?filename=toolkit%2Fcomponents%2Fextensions%2Fext-webNavigation.js and all the dependencies done. If we aim for 100%+ one day, we can revisit this.