Code review comment for lp:~diegosarmentero/ubuntu-sso-client/reset-password-page

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

* ubuntu_sso/qt/tests/test_reset_password.py:
    82: [W0212, SetupAccountTestCase.test_focus_changed_1] Access to a protected member _called of a client class
    85: [W0212, SetupAccountTestCase.test_focus_changed_1] Access to a protected member _called of a client class
    93: [W0212, SetupAccountTestCase.test_focus_changed_2] Access to a protected member _called of a client class
    96: [W0212, SetupAccountTestCase.test_focus_changed_2] Access to a protected member _called of a client class

* Question: should showEvent and hideEvent call super()?

* Silly fix: ubuntu_sso/qt/tests/test_common.py should have you as author, not mandel :-). Same for ubuntu_sso/qt/tests/test_reset_password.py.

* This is not valid style for our project!

+from ubuntu_sso.qt.common import (check_as_invalid,
+ check_as_valid,
+ password_assistance,
+ password_check_match,
+ BAD,
+ GOOD,
+ NORMAL,
+ PASSWORD_DIGIT,
+ PASSWORD_LENGTH,
+ PASSWORD_MATCH,
+ PASSWORD_UPPER)
+

it should be:

+from ubuntu_sso.qt.common import (check_as_invalid,
+ check_as_valid,
+ password_assistance,
+ password_check_match,
+ BAD,
+ GOOD,
+ NORMAL,
+ PASSWORD_DIGIT,
+ PASSWORD_LENGTH,
+ PASSWORD_MATCH,
+ PASSWORD_UPPER,
+)

* I advice to use twisted TestCase instead of unittest's.

* I think there is no need to cut off some sentences like:

+ password_assistance(line_edit,
+ label_assistance)

can you please check the rest?

* Can we have this moved to a setUp? seems to be used in every test:

+ line_edit = QtGui.QLineEdit()
+ label_assistance = QtGui.QLabel()

* This two should be together by alphabetical order:

+from PyQt4 import QtCore, QtGui
+from twisted.trial.unittest import TestCase

It looks really good! Big applause for all the tests!

review: Needs Fixing

« Back to merge proposal