Closed Bug 1190329 Opened 6 years ago Closed 4 years ago

Test coverage for webNavigation extension API

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gkrizsanits, Assigned: rpl)

References

Details

(Whiteboard: [webNavigation] triaged)

Attachments

(1 obsolete file)

Blocks: 1185459
Attached patch patch (obsolete) — Splinter Review
This just copies the test we have for the .jsm.
Attachment #8689834 - Flags: review?(kmaglione+bmo)
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?
Attachment #8689834 - Flags: review?(kmaglione+bmo) → review+
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Flags: blocking-webextensions-
Flags: blocking-webextensions- → blocking-webextensions+
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?
Assignee: nobody → 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 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
Attachment #8689834 - Attachment is obsolete: true
Whiteboard: [webNavigation] triaged
Flags: blocking-webextensions+ → blocking-webextensions-
Depends on: 1246125
Depends on: 1262794
So close, 97.26% https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-webNavigation.js.html
Component: WebExtensions: Untriaged → WebExtensions: General
Flags: blocking-webextensions-
Priority: -- → P3
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.