Code review comment for lp:~nataliabidart/ubuntu-sso-client/unify-signal-broadcaster

Major branch!!! here are some things I'v noticed:

* We do the following several times:
'SSOLogin: emitting CaptchaGenerated with app_name "%s" and result %r', app_name, result
'SSOLogin: emitting CaptchaGenerationError with app_name "%s" and error %r', app_name, error

is it possible to be smarter regarding the logging of the DBusSignals? an idea would be to use a decorator that way we know the function name and the args. I think it will remove some noise form the code.

* I'm confused, why do we have the following?:

1218 + d.addCallback(lambda a: reply_handler(*a))

If a is not a sequence we are going to have a runtime error..

* There are several tests of the type:

2869 + def test_credentials_cleared(self):
2870 + """The CredentialsCleared signal."""
2871 + self.obj.CredentialsCleared(APP_NAME)
2872 + msgs = (self.obj.__class__.__name__,
2873 + self.obj.CredentialsCleared.__name__, APP_NAME)
2874 + self.assertTrue(self.memento.check_info(*msgs))

Where we call a signal, build the msg and assert, can those be merged?

There seems to be some repetitive code here:

2583 + def test_credentials_found_when_ref_count_is_not_positive(self):
2584 + """Ref counter is decreased when a signal is sent."""
2585 + self.obj._ref_count = -3
2586 + self.obj.CredentialsFound(APP_NAME, TOKEN)
2587 +
2588 + self.assertEqual(self.obj.ref_count, 0)
2589 + msg = 'Attempting to decrease ref_count to a negative value (-4).'
2590 + self.assertTrue(self.memento.check_warning(msg))
2591 +
2592 + def test_credentials_not_found_when_ref_count_is_not_positive(self):
2593 + """Ref counter is decreased when a signal is sent."""
2594 + self.obj._ref_count = -3
2595 + self.obj.CredentialsNotFound(APP_NAME)
2596 +
2597 + self.assertEqual(self.obj.ref_count, 0)
2598 + msg = 'Attempting to decrease ref_count to a negative value (-4).'
2599 + self.assertTrue(self.memento.check_warning(msg))
2600 +
2601 + def test_credentials_cleared_when_ref_count_is_not_positive(self):
2602 + """Ref counter is decreased when a signal is sent."""
2603 + self.obj._ref_count = -3
2604 + self.obj.CredentialsCleared(APP_NAME)
2605 +
2606 + self.assertEqual(self.obj.ref_count, 0)
2607 + msg = 'Attempting to decrease ref_count to a negative value (-4).'
2608 + self.assertTrue(self.memento.check_warning(msg))
2609 +
2610 + def test_credentials_stored_when_ref_count_is_not_positive(self):
2611 + """Ref counter is decreased when a signal is sent."""
2612 + self.obj._ref_count = -3
2613 + self.obj.CredentialsStored(APP_NAME)
2614 +
2615 + self.assertEqual(self.obj.ref_count, 0)
2616 + msg = 'Attempting to decrease ref_count to a negative value (-4).'
2617 + self.assertTrue(self.memento.check_warning(msg))
2618 +
2619 + def test_credentials_error_when_ref_count_is_not_positive(self):
2620 + """Ref counter is decreased when a signal is sent."""
2621 + self.obj._ref_count = -3
2622 + self.obj.CredentialsError(APP_NAME, SampleException('test'))
2623 +
2624 + self.assertEqual(self.obj.ref_count, 0)
2625 + msg = 'Attempting to decrease ref_count to a negative value (-4).'
2626 + self.assertTrue(self.memento.check_warning(msg))
2627 +
2628 + def test_autorization_denied_when_ref_count_is_not_positive(self):
2629 + """Ref counter is decreased when a signal is sent."""
2630 + self.obj._ref_count = -3
2631 + self.obj.AuthorizationDenied(APP_NAME)
2632 +
2633 + self.assertEqual(self.obj.ref_count, 0)
2634 + msg = 'Attempting to decrease ref_count to a negative value (-4).'
2635 + self.assertTrue(self.memento.check_warning(msg))

It seems that the diff is the signal that is called. The same applies to:

2541 + def test_credentials_found(self):
2542 + """Ref counter is decreased when a signal is sent."""
2543 + self.obj.ref_count = 3
2544 + self.obj.CredentialsFound(APP_NAME, TOKEN)
2545 +
2546 + self.assertEqual(self.obj.ref_count, 2)
2547 +
2548 + def test_credentials_not_found(self):
2549 + """Ref counter is decreased when a signal is sent."""
2550 + self.obj.ref_count = 3
2551 + self.obj.CredentialsNotFound(APP_NAME)
2552 +
2553 + self.assertEqual(self.obj.ref_count, 2)
2554 +
2555 + def test_credentials_cleared(self):
2556 + """Ref counter is decreased when a signal is sent."""
2557 + self.obj.ref_count = 3
2558 + self.obj.CredentialsCleared(APP_NAME)
2559 +
2560 + self.assertEqual(self.obj.ref_count, 2)
2561 +
2562 + def test_credentials_stored(self):
2563 + """Ref counter is decreased when a signal is sent."""
2564 + self.obj.ref_count = 3
2565 + self.obj.CredentialsStored(APP_NAME)
2566 +
2567 + self.assertEqual(self.obj.ref_count, 2)
2568 +
2569 + def test_credentials_error(self):
2570 + """Ref counter is decreased when a signal is sent."""
2571 + self.obj.ref_count = 3
2572 + self.obj.CredentialsError(APP_NAME, SampleException('test'))
2573 +
2574 + self.assertEqual(self.obj.ref_count, 2)
2575 +
2576 + def test_authorization_denied(self):
2577 + """Ref counter is decreased when a signal is sent."""
2578 + self.obj.ref_count = 3
2579 + self.obj.AuthorizationDenied(APP_NAME)
2580 +
2581 + self.assertEqual(self.obj.ref_count, 2)