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.
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.
Sign in to reply to this message.
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.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
