Closed Bug 1246730 Opened 4 years ago Closed 4 years ago

Add schema test.json

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/ec234b78ab0a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.