Created attachment 33366 [details] [review]
Move _mcd_dispatcher_add_connection to an earlier stage.

Created attachment 33369 [details] [review]
Move _mcd_dispatcher_add_connection to an earlier stage.

Where's the git branch? Referencing gitweb is generally easier than attaching a series of diffs.

This functionality needs a regression test.

Please add the 'patch' keyword when this is ready for review.

Since it was such a small change, I thought it would be easier to review as a patch. Here is the git branch:

http://git.collabora.co.uk/?p=user/eitan/telepathy-mission-control.git;a=summary

I just looked at the regression tests, and it's less than trivial, so once I learn how to write new tests, I'll do that and tag this report as 'patch'.

Added a regression test, adjusted current tests, and made amendments where tests were sincerely failing.

I'll have a look, thanks.

(Telepathy convention is usually to put the thing to be reviewed in the URL field of the bug, btw.)

+    if (!priv->dispatching_started)
     _mcd_dispatcher_add_connection (priv->dispatcher, connection);

Indentation, please.

+    e = q.expect('dbus-signal', signal='StatusChanged')
+
+    # Connection should be in "Connecting" status for this test.
+    assert e.args == [cs.CONN_STATUS_CONNECTING, cs.CONN_STATUS_REASON_REQUESTED]

This seems suspicious: this signal was emitted by the SimulatedConnection, which is part of your test code, so you shouldn't have to expect() it. You can expect() a call to Connect() instead?

The code changes look good, though.

(In reply to comment #7)
> +    if (!priv->dispatching_started)
>      _mcd_dispatcher_add_connection (priv->dispatcher, connection);
> 
> Indentation, please.

That file is very confusing. Right above that statement is a similar one with a tab indentation. Anyway, I fixed it to what I believe it should be in that file.

> 
> +    e = q.expect('dbus-signal', signal='StatusChanged')
> +
> +    # Connection should be in "Connecting" status for this test.
> +    assert e.args == [cs.CONN_STATUS_CONNECTING,
> cs.CONN_STATUS_REASON_REQUESTED]
> 
> This seems suspicious: this signal was emitted by the SimulatedConnection,
> which is part of your test code, so you shouldn't have to expect() it. You can
> expect() a call to Connect() instead?

Yeah, that's stupid. Fixed.

> 
> The code changes look good, though.
> 

You still seem to be expect()ing StatusChanged. Why? The test script has full control over the SimulatedConnection's status, so this makes no sense.

Tests should be making expectations/assertions about what the code under test (MC) does, not about what the test harness does. The SimulatedConnection, and all other Python code in test/twisted/, is part of the test harness.

As I said previously, one event that it *would* make sense to expect() is the call to Connect(), which MC will do. This happens to cause StatusChanged as a side-effect of the SimulatedConnection's implementation of Connect.

(In reply to comment #9)
> You still seem to be expect()ing StatusChanged. Why? The test script has full
> control over the SimulatedConnection's status, so this makes no sense.
> 

I left it there purely for readability reasons, the channel is dispatched when the connection is in Connecting status.

> As I said previously, one event that it *would* make sense to expect() is the
> call to Connect(), which MC will do. This happens to cause StatusChanged as a
> side-effect of the SimulatedConnection's implementation of Connect.
> 

I'll use that instead, should answer my goal above.

This appears to be fixed in master.