search

OWASP / phpsec

OWASP PHP Security Project - THIS PROJECT IS INACTIVE AND MAY CONTAIN SECURITY FLAWS
197 stars 103 forks source link

Modified / Added tearDown() function for different tests #92

Closed mebjas closed 10 years ago

mebjas commented 10 years ago

There were certain tests for which objects weren't being destroyed after tests are over. added tearDown() in logging library test

SvenRtbg commented 10 years ago

Trink about this: An object that only resides in memory and will get overwritten before the next test method is run by setUp() does not need to be unset. This is considered bad practice.

Teardown is there to get rid of left over things that are not in volatile memory, like database entries.

But then again do you really want to drop an entire test table before every test function and then create it again? Think about the performance penalty. Think about test cases that do test in multiple steps.

And then think about the TravisCI setup: There is no permanent database. So only for this scenario there is no need for cleanup even in the database.

mebjas commented 10 years ago

Its mentioned in phpunit's website: if you create many objects in your setUp(), you might want to unset() the variables pointing to those objects in your tearDown() so they can be garbage collected. The garbage collection of test case objects is not predictable.

Its a common practice in phpmyadmin so I thought it would be great to practise this since beginning in phpsec.

SvenRtbg commented 10 years ago

Can you point me to a source for that? Because I remember reading there shouldn't be a need for teardown in general unless you really have persistent test garbage to remove.

On 4. April 2014 21:50:03 MESZ, minhaz notifications@github.com wrote:

Its mentioned id phpunit's website: if you create many objects in your setUp(), you might want to unset() the variables pointing to those objects in your tearDown() so they can be garbage collected. The garbage collection of test case objects is not predictable.

Its a common practice in phpmyadmin so I thought it would be great to practise this since beginning in phpsec.

Reply to this email directly or view it on GitHub: https://github.com/OWASP/phpsec/pull/92#issuecomment-39604273

mebjas commented 10 years ago

Its mentioned in phpunit website only @ http://phpunit.de/manual/current/en/fixtures.html Also what you have said is correct tearDown() is needed in such cases only ....

but what I'm suggesting is isn't it advisable to tear down objects/object pointers used as the project grows bigger?

SvenRtbg commented 10 years ago

The test class only has one instance of all the objects. Setup overwrites them constantly. There is no need to explicitly unset them before.

The only case where they are not again overwritten is when the last test method finished. Only then would it be a difference if there is no unsetting. But PHPUnit will unset the test class after the tests in it have run. And this will unset these objects as well.

So doing such housekeeping really serves no purpose, but it adds to the test codebase and has to be maintained. And distracts from the tests. It also does not really keep memory consumption down. PHPUnit will happily use 1 GB and more if the project is huge, there are plenty of tests and code coverage has to be generated. There is no way to avoid this. Deleting objects manually probably has no effect here (the only reasonable exception would be using PHP 5.2 which cannot delete circular references well - but you shouldn't really create such circles in a unit test).