test_Domains.js fails with `test_Domains_splitMethod - [test_Domains_splitMethod : 126] ["",""] deepEqual []`

RESOLVED FIXED

Status

enhancement
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: ochameau, Assigned: ato)

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

3 months ago

$ ./mach test remote/test/unit/test_Domains.js

Will result in:

remote/test/unit/test_Domains.js
FAIL remote/test/unit/test_Domains.js - xpcshell return code: 0
FAIL test_Domains_splitMethod - [test_Domains_splitMethod : 126] ["",""] deepEqual []
/mnt/desktop/gecko-dev/obj-firefox-artifact/_tests/xpcshell/remote/test/unit/test_Domains.js:test_Domains_splitMethod:126

Assignee

Comment 1

3 months ago

I will fix.

Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee

Comment 2

3 months ago

Instead of coercing si to a true-ish value to decide when to break
the loop, we should make sure to check it is explicitly in the
negative range. This ensures that si = 0 evalutes to true.

Fixes the following test in remote/test/unit/test_Domains.js:

FAIL test_Domains_splitMethod - [test_Domains_splitMethod : 126] ["",""] deepEqual []

We can't have enough off-by-one bugs.

Assignee

Comment 3

3 months ago

Fixes the "Foo.bar.baz" test case in remote/test/unit/test_Domains.js.

Assignee

Comment 4

3 months ago

The remote agent currently uses "method" interchangably for the
full method string as extracted from JSON input as well as for the
function part following the first dot after the method has been split.

To avoid namespace clashes, this patch makes a distinction between
method, being the input JSON field; the first substring prior to the
dot being the domain; and the rest that follows being called the command:

method = "<domain>.<command>"

This naming seems to be supported by chrome-remote-interface:

https://github.com/cyrus-and/chrome-remote-interface/blob/master/lib/api.js#L32
Assignee

Comment 5

3 months ago

This moves the assertions related to the well-formedness of the method
from the TabSession consumer to Domains.splitMethod. Following the
introduction of TabSession, this was missing from the superclass Session.

This also fixes the "Foo.bar.baz" test case in
remote/test/unit/test_Domains.js by removing the split() function
and instead relying on String#split() inside Domains.splitMethod.

Thanks-to: Alexandre Poirot <ochameau@mozilla.com>

Attachment #9054870 - Attachment is obsolete: true
Attachment #9054871 - Attachment is obsolete: true

Comment 6

3 months ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28eb8a6b33f6
remote: clarify method/domain/command terminology; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/2bccb12b0ee6
remote: move method sanity check into Domains.splitMethod; r=ochameau

Comment 7

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.