1114432 - mozdevice forward raise exception cause MCTS failed

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Certification Suite, defect)

Description

[oo[oouyang]]

Bug 1094164 - [mozdevice] Port forward will fail if the socket is already bound. (edit)
Before the bug 1094164 landing, forward return 0 or 1 to indicate failure or success of forward action.
After landing, it cause MCTS failed.

Comment 1

[oo[oouyang]]

Attachment: bug1114432.diff

Please help to review the code

Comment 2

[oo[oouyang]]

Attachment: bug1114432.diff

Comment 3

[oo[oouyang]]

Attachment: bug1114432.patch

Comment 4

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8543963 [details] [diff] [review]
bug1114432.patch

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

This will still cause us to thrown an exception when dm.forward fails, and I'm not sure that returning the error code is useful.  Instead, I suggest we wrap call sites with try/except so we can handle the exception.

Comment 5

[oo[oouyang]]

Comment on attachment 8543963 [details] [diff] [review]
bug1114432.patch

We need this function return 0 when forward successfully for MCTS.
In this fix, if it forwarded failed, it throws a DMError, but in old code it return not 0, and MCTS throws another exception. The situation is almost the same what we expected.

If it forwarded successfully, it return 0 and continue MCTS. And in old code it return None and cause exception because None is not 0 and MCTS cannot continue.
After the fix, we expected the MCTS could continue when it forwards successfully.

I think this meet MCTS needs and not break the original fix to throw DMError when forwarding failed.

Comment 6

[Jonathan Griffin (:jgriffin)]

At issue are blocks of code in MCTS like this:

https://github.com/mozilla-b2g/fxos-certsuite/blob/v2.0/certsuite/cert.py#L384

if dm.forward("tcp:2828", "tcp:2828") != 0:
    raise Exception("Can't use localhost:2828 for port forwarding." \
    "Is something else using port 2828?")

We could instead change this to:

try:
    dm.forward("tcp:2828", "tcp:2828")
except DMError:
    raise Exception("Can't use localhost:2828 for port forwarding." \
    "Is something else using port 2828?")

I think this is better than changing DeviceManagerADB to return 0 in the passing case, from a design perspective.