Issue 2214046: Unit Tests for ModelSQL read method and update to test module
DescriptionI have added tests and there are 4 tests which fail. The reasons have been tagged with FIXME in the test file
The tests have a coverage of 96.15% of the read method.
The unhandled cases are:
Lines 590-596 (Will never reach there because function field spec does not allow the use of datetime_field by design. So no use in read method)
Line 657,
Line 660 - Need to enter invalid model_name in reference field,
Line 663 - Need to enter illegal_id in reference field
Line 687 - Need to enter model_name in reference field,
Line 690 - Need to enter illegal_id in reference field
Patch Set 1 #
Total comments: 60
Patch Set 2 : Changes adviced by cedk #
Total comments: 20
Patch Set 3 : All object lookups moved to setup, uses assertEqual and assertFalse instead of pythonic comparison #
Total comments: 1
Patch Set 4 : Added the tests missed in the previous push #
Total comments: 6
Patch Set 5 : Added tests for language and fixed issue pointed by cedk #Patch Set 6 : Fixed a line which had more than 80 chars #
Total comments: 3
MessagesTotal messages: 18
http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py File trytond/test/model_sql.py (right): http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode8 trytond/test/model_sql.py:8: class SQLModel(ModelSQL): Name the models with Read inside because they are used for read test http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode66 trytond/test/model_sql.py:66: 'MIN(COALESCE("%s".write_date, "%s".create_date)) AS date' % (table, table), 80cols http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode69 trytond/test/model_sql.py:69: query = ('SELECT ' + ', '.join(fields) + ' from') + \ upper case for SQL http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode69 trytond/test/model_sql.py:69: query = ('SELECT ' + ', '.join(fields) + ' from') + \ Can use parenthesis instead of "\" http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode87 trytond/test/model_sql.py:87: InheritsModel() Space before instance http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:12: class ModelSQLTestCase(unittest.TestCase): I think it is better to name the test case with only read inside. And the file is already big just for read test http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:29: self.assertRaises(Exception, self.sql_model.read, 999999) You can use -1 which is sure to not exist http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:40: self.assert_(type(self.sql_model.read(create_id))==dict) space around == http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:40: self.assert_(type(self.sql_model.read(create_id))==dict) Can't you test also the returned values? http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:41: self.assert_(type(self.sql_model.read([create_id]))==list) space around == http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:58: data_1 = {'field_char':'Something'} space after : http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:76: def test0035read(self): As it is the initial version, you could renumber the tests http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:98: # not have this field at all Or it should raise an exception Because I don't think reference field should work like many2one. I doesn't in BrowseRecord http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:114: 'field_int':1, space after : http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:125: read_data['field_m2o.field_char']==data_1['field_char'] space around == http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:128: read_data = self.sql_model2.read( Don't test read_data? http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:141: read_data['field_m2o_simple.field_char']==data_1['field_char'] space around == http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:158: data_1 = {'field_char':'Something'} space around : http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:163: 'field_int':101, space after : http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:172: self.assert_(len(read_data.keys())==2) space around == http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:180: self.assert_(read_data['field_m2o.field_char']=='Something') space around == http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:184: for key, value in data.items(): You should also test with data_1 http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:185: self.assert_(read_data[key]==value) space around == http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:248: ids = [1] Where does 1 come? http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:250: self.sql_model_history.read(ids) You should test read data http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:258: data = {'field_char':'Something', 'field_boolean':False} space after : and better to put it on multilines http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:273: 'name':'test Group', space after : http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:274: 'model':ir_model_obj.search([('model', '=', 'test.sqlmodel')])[0], 80cols http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:280: 'field':ir_field_obj.search( More readable if you put search outside the rule {} definition
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py File trytond/test/model_sql.py (right): http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode70 trytond/test/model_sql.py:70: (' %s' % table) You must group by something.
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py File trytond/test/model_sql.py (right): http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode70 trytond/test/model_sql.py:70: (' %s' % table) On 2010/09/25 10:07:20, ced wrote: > You must group by something. Or don't use MIN etc.
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py File trytond/test/model_sql.py (right): http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode8 trytond/test/model_sql.py:8: class SQLModel(ModelSQL): On 2010/09/25 08:04:28, ced wrote: > Name the models with Read inside because they are used for read test Done. http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode66 trytond/test/model_sql.py:66: 'MIN(COALESCE("%s".write_date, "%s".create_date)) AS date' % (table, table), On 2010/09/25 08:04:28, ced wrote: > 80cols Done. http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode66 trytond/test/model_sql.py:66: 'MIN(COALESCE("%s".write_date, "%s".create_date)) AS date' % (table, table), On 2010/09/25 08:04:28, ced wrote: > 80cols Done. http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode69 trytond/test/model_sql.py:69: query = ('SELECT ' + ', '.join(fields) + ' from') + \ On 2010/09/25 08:04:28, ced wrote: > upper case for SQL Done. http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode69 trytond/test/model_sql.py:69: query = ('SELECT ' + ', '.join(fields) + ' from') + \ On 2010/09/25 08:04:28, ced wrote: > Can use parenthesis instead of "\" Done. http://codereview.appspot.com/2214046/diff/1/trytond/test/model_sql.py#newcode87 trytond/test/model_sql.py:87: InheritsModel() On 2010/09/25 08:04:28, ced wrote: > Space before instance Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:12: class ModelSQLTestCase(unittest.TestCase): On 2010/09/25 08:04:28, ced wrote: > I think it is better to name the test case with only read inside. > And the file is already big just for read test Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:29: self.assertRaises(Exception, self.sql_model.read, 999999) On 2010/09/25 08:04:28, ced wrote: > You can use -1 which is sure to not exist Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:40: self.assert_(type(self.sql_model.read(create_id))==dict) On 2010/09/25 08:04:28, ced wrote: > space around == Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:40: self.assert_(type(self.sql_model.read(create_id))==dict) On 2010/09/25 08:04:28, ced wrote: > Can't you test also the returned values? Tested right below :) http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:41: self.assert_(type(self.sql_model.read([create_id]))==list) On 2010/09/25 08:04:28, ced wrote: > space around == Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:58: data_1 = {'field_char':'Something'} On 2010/09/25 08:04:28, ced wrote: > space after : Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:76: def test0035read(self): On 2010/09/25 08:04:28, ced wrote: > As it is the initial version, you could renumber the tests Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:114: 'field_int':1, On 2010/09/25 08:04:28, ced wrote: > space after : Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:125: read_data['field_m2o.field_char']==data_1['field_char'] On 2010/09/25 08:04:28, ced wrote: > space around == Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:128: read_data = self.sql_model2.read( On 2010/09/25 08:04:28, ced wrote: > Don't test read_data? Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:141: read_data['field_m2o_simple.field_char']==data_1['field_char'] On 2010/09/25 08:04:28, ced wrote: > space around == Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:158: data_1 = {'field_char':'Something'} On 2010/09/25 08:04:28, ced wrote: > space around : Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:163: 'field_int':101, On 2010/09/25 08:04:28, ced wrote: > space after : Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:172: self.assert_(len(read_data.keys())==2) On 2010/09/25 08:04:28, ced wrote: > space around == Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:180: self.assert_(read_data['field_m2o.field_char']=='Something') On 2010/09/25 08:04:28, ced wrote: > space around == Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:184: for key, value in data.items(): On 2010/09/25 08:04:28, ced wrote: > You should also test with data_1 Tested below http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:185: self.assert_(read_data[key]==value) On 2010/09/25 08:04:28, ced wrote: > space around == Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:248: ids = [1] On 2010/09/25 08:04:28, ced wrote: > Where does 1 come? Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:250: self.sql_model_history.read(ids) On 2010/09/25 08:04:28, ced wrote: > You should test read data Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:258: data = {'field_char':'Something', 'field_boolean':False} On 2010/09/25 08:04:28, ced wrote: > space after : > and better to put it on multilines Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:273: 'name':'test Group', On 2010/09/25 08:04:28, ced wrote: > space after : Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:274: 'model':ir_model_obj.search([('model', '=', 'test.sqlmodel')])[0], On 2010/09/25 08:04:28, ced wrote: > 80cols Done. http://codereview.appspot.com/2214046/diff/1/trytond/tests/test_modelsql.py#n... trytond/tests/test_modelsql.py:280: 'field':ir_field_obj.search( On 2010/09/25 08:04:28, ced wrote: > More readable if you put search outside the rule {} definition Done.
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py File trytond/test/model_sql.py (right): http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py#new... trytond/test/model_sql.py:51: class SQLReadModelHistory(ModelSQL): No more used http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py#new... trytond/test/model_sql.py:66: 'MIN(COALESCE("%s".write_date, "%s".create_date)) AS date' % ( Don't need of MIN http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py#new... trytond/test/model_sql.py:71: (' %s' % table)) Why not: ' FROM "%s"' % table btw: need " around table http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:23: self.sql_model_history = POOL.get('test.read_sqlmodel.history') no more used http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:108: ir_model_obj = POOL.get('ir.model') Can be put in setup http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:151: self.assert_( Perhaps it is better to use assertEqual http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:261: '%s.__id AS id' % table, quote around table http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:265: ('SELECT ') + no need of () http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:267: (' FROM %s' % table) quote around table http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:298: ir_rule_obj = POOL.get('ir.rule') Could be defined in setup
Sign in to reply to this message.
Updated code will follow http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py File trytond/test/model_sql.py (right): http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py#new... trytond/test/model_sql.py:51: class SQLReadModelHistory(ModelSQL): On 2010/09/25 11:31:24, ced wrote: > No more used Now used :) http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py#new... trytond/test/model_sql.py:66: 'MIN(COALESCE("%s".write_date, "%s".create_date)) AS date' % ( On 2010/09/25 11:31:24, ced wrote: > Don't need of MIN Done. http://codereview.appspot.com/2214046/diff/8001/trytond/test/model_sql.py#new... trytond/test/model_sql.py:71: (' %s' % table)) On 2010/09/25 11:31:24, ced wrote: > Why not: > > ' FROM "%s"' % table > > btw: need " around table Done. http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:23: self.sql_model_history = POOL.get('test.read_sqlmodel.history') On 2010/09/25 11:31:24, ced wrote: > no more used Done. http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:108: ir_model_obj = POOL.get('ir.model') On 2010/09/25 11:31:24, ced wrote: > Can be put in setup Done. http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:151: self.assert_( On 2010/09/25 11:31:24, ced wrote: > Perhaps it is better to use assertEqual Done. http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:261: '%s.__id AS id' % table, On 2010/09/25 11:31:24, ced wrote: > quote around table Done. http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:265: ('SELECT ') + On 2010/09/25 11:31:24, ced wrote: > no need of () Done. http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:267: (' FROM %s' % table) On 2010/09/25 11:31:24, ced wrote: > quote around table Done. http://codereview.appspot.com/2214046/diff/8001/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:298: ir_rule_obj = POOL.get('ir.rule') On 2010/09/25 11:31:24, ced wrote: > Could be defined in setup Done.
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/14001/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/14001/trytond/tests/test_modelsql.... trytond/tests/test_modelsql.py:283: read_data = self.sql_model_history.read(ids) check the read data
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:270: transaction.cursor.execute(select_query) In fact, here you only test create and write history. I think it should go on other tests. http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:287: self.assertEqual(read_data[0]['field_char'], data['field_char']) How can you be sure about the order?
Sign in to reply to this message.
Please suggest http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:270: transaction.cursor.execute(select_query) On 2010/09/25 12:49:29, ced wrote: > In fact, here you only test create and write history. > I think it should go on other tests. I think these should go to test0070read where history is tested? http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:287: self.assertEqual(read_data[0]['field_char'], data['field_char']) On 2010/09/25 12:49:29, ced wrote: > How can you be sure about the order? By default the order will be on ID? happy to change to a better way :)
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.py File trytond/tests/test_modelsql.py (right): http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:270: transaction.cursor.execute(select_query) On 2010/09/25 12:52:25, sharoonthomas wrote: > On 2010/09/25 12:49:29, ced wrote: > > In fact, here you only test create and write history. > > I think it should go on other tests. > > I think these should go to test0070read where history is tested? No sure it is low level check. But test0070read could be improved to test with different datetime http://codereview.appspot.com/2214046/diff/6002/trytond/tests/test_modelsql.p... trytond/tests/test_modelsql.py:287: self.assertEqual(read_data[0]['field_char'], data['field_char']) On 2010/09/25 12:52:25, sharoonthomas wrote: > On 2010/09/25 12:49:29, ced wrote: > > How can you be sure about the order? > > By default the order will be on ID? happy to change to a better way :) Yes but not sure that the ids are increasing. Perhaps sort on date
Sign in to reply to this message.
http://codereview.appspot.com/2214046/diff/8003/trytond/test/model_sql.py File trytond/test/model_sql.py (right): http://codereview.appspot.com/2214046/diff/8003/trytond/test/model_sql.py#new... trytond/test/model_sql.py:5: "Test Models for testing Model SQL" "Test Models for testing class ModelSQL" http://codereview.appspot.com/2214046/diff/8003/trytond/test/model_sql.py#new... trytond/test/model_sql.py:20: field_char_trans = fields.Char('Char field with translate', translate=True) field_translate is also Char and translate=True field_char_trans doesn't seem to be used later in tests. http://codereview.appspot.com/2214046/diff/8003/trytond/test/model_sql.py#new... trytond/test/model_sql.py:97: "An Model with 2 _inherits arrangement - Tests Read" A Model
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
