Tests don't call super's tearDownClass #161

Closed
atodorov opened this Issue Feb 28, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@atodorov
Contributor

atodorov commented Feb 28, 2017

During work on #159 I have discovered that many tests (not only in xmlrpc app) don't call tearDownClass() of the parent class. This may be a potential issue and is also a requirement in PR review so my vote goes for fixing this everywhere and set an example.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 1, 2017

Member

Yup, this should really be fixed. Are you fixing or do u want to fix?

Member

tkdchen commented Mar 1, 2017

Yup, this should really be fixed. Are you fixing or do u want to fix?

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 1, 2017

Contributor

I can take a look at this at the end of the week, it's not high prio. Can you assign it to me ?

Contributor

atodorov commented Mar 1, 2017

I can take a look at this at the end of the week, it's not high prio. Can you assign it to me ?

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 2, 2017

Contributor

So I found few interesting data points regarding this issue:
https://docs.djangoproject.com/en/1.10/topics/testing/tools/#django.test.TestCase.setUpTestData

  1. The data setup function should be setUpTestData as given in the docs, no need to call super() here. If I read django/tests/testcases.py correctly, it's setUpClass calls setUpTestData inside a transaction. This means that our tests at present don't benefit of this new mechanism.

  2. As far as I can tell there's no need to delete any objects created in setUpClass. After Django 1.8 (ticket https://code.djangoproject.com/ticket/20392 I believe) the test cases are executed into a DB transaction and the DB objects created in setup are dropped.

  3. sqlite, which is used for testing, supports transactions which further solidifies 1). However MySQL with the MyISAM engine does not support transactions. So far we're not testing with MySQL but I need to check what are the defaults with latest versions of MySQL.

  4. Due to the transactional nature of the test runner, see 1), there may be some problems with code which also tries to work with transactions. I've read somewhere (no official Django doc) that begin/end transaction will not be execute while running the tests. This may interfere with the changes from #162.

  5. This very same transactional behavior may be the source of coredumps we're seeing in Travis-CI since #159 got merged.

I still can't reproduce this locally but I'm changing and commenting out stuff and pushing it to Travis to see what happens. So far the only thing that didn't cause a core dump was commenting out tearDownClass on xmlrpc/tests/test_testcase.py:TestNotificationRemoveCC. I will continue to experiment over the next following days.

Contributor

atodorov commented Mar 2, 2017

So I found few interesting data points regarding this issue:
https://docs.djangoproject.com/en/1.10/topics/testing/tools/#django.test.TestCase.setUpTestData

  1. The data setup function should be setUpTestData as given in the docs, no need to call super() here. If I read django/tests/testcases.py correctly, it's setUpClass calls setUpTestData inside a transaction. This means that our tests at present don't benefit of this new mechanism.

  2. As far as I can tell there's no need to delete any objects created in setUpClass. After Django 1.8 (ticket https://code.djangoproject.com/ticket/20392 I believe) the test cases are executed into a DB transaction and the DB objects created in setup are dropped.

  3. sqlite, which is used for testing, supports transactions which further solidifies 1). However MySQL with the MyISAM engine does not support transactions. So far we're not testing with MySQL but I need to check what are the defaults with latest versions of MySQL.

  4. Due to the transactional nature of the test runner, see 1), there may be some problems with code which also tries to work with transactions. I've read somewhere (no official Django doc) that begin/end transaction will not be execute while running the tests. This may interfere with the changes from #162.

  5. This very same transactional behavior may be the source of coredumps we're seeing in Travis-CI since #159 got merged.

I still can't reproduce this locally but I'm changing and commenting out stuff and pushing it to Travis to see what happens. So far the only thing that didn't cause a core dump was commenting out tearDownClass on xmlrpc/tests/test_testcase.py:TestNotificationRemoveCC. I will continue to experiment over the next following days.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 2, 2017

Contributor

Adding a note about core dumps in Travis CI b/c they may be related (not sure ATM). It also looks like there is a problem with older versions of sqlite3, see pytest-dev/pytest-django#409

My local version if 3.7.17 2013-05-20 00:56:22 118a3b35693b134d56ebd780123b7fd6f1497668 while Travis has 3.7.9 2011-11-01 00:52:41 c7c6050ef060877ebe77b41d959e9df13f8c9b5e.

@tkdchen what's your version of sqlite3 ?

Contributor

atodorov commented Mar 2, 2017

Adding a note about core dumps in Travis CI b/c they may be related (not sure ATM). It also looks like there is a problem with older versions of sqlite3, see pytest-dev/pytest-django#409

My local version if 3.7.17 2013-05-20 00:56:22 118a3b35693b134d56ebd780123b7fd6f1497668 while Travis has 3.7.9 2011-11-01 00:52:41 c7c6050ef060877ebe77b41d959e9df13f8c9b5e.

@tkdchen what's your version of sqlite3 ?

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 3, 2017

Member

The data setup function should be setUpTestData as given in the docs, no need to call super() here. If I read django/tests/testcases.py correctly, it's setUpClass calls setUpTestData inside a transaction. This means that our tests at present don't benefit of this new mechanism.

It's good to use setUpTestData, however it is necessary to be much careful to reload data if data is changed during tests as Django documentation mentions.

As far as I can tell there's no need to delete any objects created in setUpClass. After Django 1.8 (ticket https://code.djangoproject.com/ticket/20392 I believe) the test cases are executed into a DB transaction and the DB objects created in setup are dropped.

So, using setUpTestData covers this.

sqlite, which is used for testing, supports transactions which further solidifies 1). However MySQL with the MyISAM engine does not support transactions. So far we're not testing with MySQL but I need to check what are the defaults with latest versions of MySQL.

I don't want to run Nitrate on MyISAM. For MySQL, it's InnoDB. What do you mean by "what are the defaults with latest versions of MySQL."?

Due to the transactional nature of the test runner, see 1), there may be some problems with code which also tries to work with transactions. I've read somewhere (no official Django doc) that begin/end transaction will not be execute while running the tests. This may interfere with the changes from #162.

I think this should be fine to #162, since in PR #167, transaction.atomic is removed because it is not necessary indeed. However, in Nitrate, many views don't run and modify data within a transaction, but it really requires transaction. After changing those views to make them run within a transcation, this item would affect tests to test those views.

Member

tkdchen commented Mar 3, 2017

The data setup function should be setUpTestData as given in the docs, no need to call super() here. If I read django/tests/testcases.py correctly, it's setUpClass calls setUpTestData inside a transaction. This means that our tests at present don't benefit of this new mechanism.

It's good to use setUpTestData, however it is necessary to be much careful to reload data if data is changed during tests as Django documentation mentions.

As far as I can tell there's no need to delete any objects created in setUpClass. After Django 1.8 (ticket https://code.djangoproject.com/ticket/20392 I believe) the test cases are executed into a DB transaction and the DB objects created in setup are dropped.

So, using setUpTestData covers this.

sqlite, which is used for testing, supports transactions which further solidifies 1). However MySQL with the MyISAM engine does not support transactions. So far we're not testing with MySQL but I need to check what are the defaults with latest versions of MySQL.

I don't want to run Nitrate on MyISAM. For MySQL, it's InnoDB. What do you mean by "what are the defaults with latest versions of MySQL."?

Due to the transactional nature of the test runner, see 1), there may be some problems with code which also tries to work with transactions. I've read somewhere (no official Django doc) that begin/end transaction will not be execute while running the tests. This may interfere with the changes from #162.

I think this should be fine to #162, since in PR #167, transaction.atomic is removed because it is not necessary indeed. However, in Nitrate, many views don't run and modify data within a transaction, but it really requires transaction. After changing those views to make them run within a transcation, this item would affect tests to test those views.

@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 3, 2017

Member

My Nitrate development box is Fedora 25. sqlite version is

3.14.2 2016-09-12 18:50:49 29dbef4b8585f753861a36d6dd102ca634197bd6
Member

tkdchen commented Mar 3, 2017

My Nitrate development box is Fedora 25. sqlite version is

3.14.2 2016-09-12 18:50:49 29dbef4b8585f753861a36d6dd102ca634197bd6
@tkdchen

This comment has been minimized.

Show comment
Hide comment
@tkdchen

tkdchen Mar 3, 2017

Member

Regarding the tests, running in sqlite is a nice way to boost the whole process, but I'm also thinking of whether it is necessary to run tests with MySQL and PostgreSQL, either in parallel or in sequence, because Nitrate should work well with those two database. What do you think?

Member

tkdchen commented Mar 3, 2017

Regarding the tests, running in sqlite is a nice way to boost the whole process, but I'm also thinking of whether it is necessary to run tests with MySQL and PostgreSQL, either in parallel or in sequence, because Nitrate should work well with those two database. What do you think?

atodorov added a commit to MrSenko/Nitrate that referenced this issue Mar 5, 2017

Convert setUpClass to setUpTestData and drop tearDownClass()
for all test cases which use the database. This speeds up things
a bit and is more compatible with how Django is supposed to handle
tests that need objects from the database.

Fix #161

atodorov added a commit to MrSenko/Nitrate that referenced this issue Mar 8, 2017

Convert setUpClass to setUpTestData and drop tearDownClass()
for all test cases which use the database. This speeds up things
a bit and is more compatible with how Django is supposed to handle
tests that need objects from the database.

Fix #161

atodorov added a commit to MrSenko/Nitrate that referenced this issue Mar 9, 2017

Convert setUpClass to setUpTestData and drop tearDownClass()
for all test cases which use the database. This speeds up things
a bit and is more compatible with how Django is supposed to handle
tests that need objects from the database.

Fix #161

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>

@tkdchen tkdchen closed this in 64830ea Mar 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment