Add schema test.json

RESOLVED FIXED in Firefox 47

Status

RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla47

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

Created attachment 8717099 [details] [diff] [review]
patch

I kinda cobbled this together from a Chromium schema and based on how we use browser.test.
Attachment #8717099 - Flags: review?(kmaglione+bmo)
Attachment #8717099 - Attachment is patch: true
Comment on attachment 8717099 [details] [diff] [review]
patch

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

::: toolkit/components/extensions/schemas/test.json
@@ +37,5 @@
> +        "description": "Sends a string message to the browser process, generating a Notification that C++ test code can wait for.",
> +        "allowAmbiguousOptionalArguments": true,
> +        "parameters": [
> +          {"type": "any", "name": "arg1", "optional": true},
> +          {"type": "any", "name": "arg2", "optional": true}

I guess we need a way to specify rest args...

@@ +62,5 @@
> +          {
> +            "name": "test",
> +            "choices": [
> +              {"type": "string"},
> +              {"type": "boolean"}

We should probably accept integers and null, too. Same for assertFalse.

Although, I wonder if checking the type is going to lead to incorrect results if a test winds up passing an object, when it's expecting something else, and the test function throws rather than reporting failure?

@@ +105,5 @@
> +        "unsupported": true,
> +        "allowAmbiguousOptionalArguments": true,
> +        "parameters": [
> +          {"type": "any", "name": "expected", "optional": true},
> +          {"type": "any", "name": "actual", "optional": true}

Should the first two args really be optional (same for assertEq)? Does type=any not allow us to pass explicit null/undefined?

@@ +129,5 @@
> +        "name": "assertLastError",
> +        "type": "function",
> +        "unsupported": true,
> +        "parameters": [
> +          {"type": "string", "name": "expectedError"}

Might be good to accept the same types as assertThrows here.

@@ +144,5 @@
> +            "name": "self",
> +            "additionalProperties": {"type": "any"},
> +            "optional": true
> +          },
> +          {"type": "array", "items": {"type": "any"}, "name": "args"},

I guess we're stuck accepting "self" and "args" if we want to match Chrome... They don't really seem necessary, though. If we're not passing a closure, we can always use .bind().

Would be nice to at least make "args" optional too.

@@ +145,5 @@
> +            "additionalProperties": {"type": "any"},
> +            "optional": true
> +          },
> +          {"type": "array", "items": {"type": "any"}, "name": "args"},
> +          {"choices": [ {"type": "string"}, {"type": "object", "isInstanceOf": "RegExp"} ], "name": "message", "optional": true}

Assert.throws allows a function that takes the error value and returns true or false, too. Not sure how useful that'd be here, though.
Attachment #8717099 - Flags: review?(kmaglione+bmo) → review+

Comment 3

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ec234b78ab0a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

6 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.