Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tearDown not executed when test fails with error or exception #254

Closed
richard-ejem opened this issue Sep 16, 2015 · 33 comments
Closed

tearDown not executed when test fails with error or exception #254

richard-ejem opened this issue Sep 16, 2015 · 33 comments
Labels

Comments

@richard-ejem
Copy link
Contributor

@richard-ejem richard-ejem commented Sep 16, 2015

I suppose this is a bug - this tidy method should be executed no matter if the test succeeds or fails. Actually tearDown works if the test fails on assertion, but not on error/exception.

//Edit: it works on exception, but not on any runtime error.

Please confirm it is a bug, I can fix and pullrequest it.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 16, 2015

I guess it's already fixed fc6d855

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 16, 2015

TearDown is not executed when PHP stops on Fatal error I think. For all other errors and exceptions it works.

@milo: Maybe add tearDown to shutdown_handler?

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 16, 2015

Confirming it does not work on errors, even E_NOTICE.
try this:

class ErroredTest extends Tester\TestCase
{
    public static $tornDown = FALSE;
    protected function tearDown() { self::$tornDown = TRUE; }
    public function testPublic() { ++$a; }
}

register_shutdown_function(function() {
    Assert::true(ErroredTest::$tornDown);
    exit(0); //to reset the exit code back from 255 after E_NOTICE killed the script
});

$test = new ErroredTest;
$test->run();

Registering teardown to shutdown function seems legit.

richard-ejem added a commit to richard-ejem/tester that referenced this issue Sep 16, 2015
@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 17, 2015

Test will fail on unexpected error. There is no need to run tearDown().

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 17, 2015

It will skip tearDown on unexpected error, but not on unexpected exception, isn't that just inconsistent?

tearDown is supposed to "tidy after the test", i.e. restore test database state, delete temporary files etc. This should be done regardless ot the test result, including termination on error.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 18, 2015

I agree witch @richard-ejem. Not executing tearDown can lead to unexpected bahaviour of following tests.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

Tests must be independent. The only reason for executing tearDown() when test method fails, used to be @throw annotation. After 710c4b5 it can be removed.

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 18, 2015

@milo what should the tearDown() method be used for by your opinion?

As i wrote: I think it should tidy database, temp files etc., which should happen regardless of test result.

So, If you say that it is not needed when test ends with error, I assume that you use tearDown for something different, that makes sense when the test succeeds, but not when it fails?

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

I'm using it for unlocking and additional assertions. All preparations should be done in setUp(), it prepares environment for testing.

On test fail, I need to preserve environment as is as much as possible.

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 18, 2015

OK, that's quite valid reason. Thanks, it's clear to me now that tearDown() makes sense only for succeeding tests to cleanup before following test methods.
And I just figured out that in case I really need to cleanup some mess regardless of test result (mentioned temp files etc.), I can use TestCase::__destruct, which executes even when failing with notice :)

Therefore I think the correct solution is to not execute teardown even when the test fails on exception, am I correct? If yes, I'll change the pull request to do that.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

I already opened #257, let's wait for David's opinion on this.

@milo milo added the won't fix label Sep 18, 2015
@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 18, 2015

OK, we may close it there, thanks ;)
(and I'm happy that I've found a valid usage of a destructor :D )

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

:)

@milo milo closed this Sep 18, 2015
@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 18, 2015

@milo I disagree with that solution.

Main reason is that all other test frameworks (PHPUnit, JUnit) execute tearDown after failing test so I think not executing tearDown after failing test can be WTF for many users.

One reason to use tearDown - sometimes only test itself knows how to properly clean up after himself. Other tests might not know how to clean up properly or only can use some perfomance killing techniques like wipe out and reinitialize entire environment.

Example of what can be done in tearDown is unlocking resources as you said. But if test lock resource and then fails file remain locked and other tests will hang up or fail after timeout.

I agree that sometimes you need to preserve enviromnemt after test fails but I think avoid execution of tearDown is not the right way.

@hrach

This comment has been minimized.

Copy link

@hrach hrach commented Sep 18, 2015

One reason to use tearDown - sometimes only test itself knows how to properly clean up after himself. Other tests might not know how to clean up properly or only can use some perfomance killing techniques like wipe out and reinitialize entire environment.

👍

In PhpUnit If I don't want to cleanup env, I modify teardown (override it to be empty, etc.).

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

In PhpUnit If I don't want to cleanup env, I modify teardown (override it to be empty, etc.).

You override it conditionally when test failed?

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

Relay on clean-up in tearDown() is false sense of safety. What would you do on fatal errors?

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 18, 2015

Well, as I found out now, when fatal error occurs, my pull request there still catches it and runs teardown :) it works even on parse error on included code.

Destructor is in general not called on fatal error, so maybe the pull request comes back to discussion as I see other people would also welcome it.

@hrach

This comment has been minimized.

Copy link

@hrach hrach commented Sep 18, 2015

You override it conditionally when test failed?

When I need something what has been cleaned up.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 18, 2015

@milo As I suggested, what about call tearDown forom shutdown_handler? It should work even for fatal errors.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

You override it conditionally when test failed?
When I need something what has been cleaned up.

@hrach You are talking about editing test source code. I'm talking about run time.

As I suggested, what about call tearDown forom shutdown_handler? It should work even for fatal errors.

@MartinMystikJonas Where would you register it in the Tester\TestCase? __construct(), setUp(), runTest()? What if there will be an exception, non autoloaded class or fatal error in tearDown()? Will be cleaning-up done or not?

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Sep 18, 2015

@richard-ejem Check out Travis.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 18, 2015

@milo I think best place is in runTest. When there is error in tearDown itself you are in situation with no solution. BUt I think tahat in any other case we should run tearDown whenever possible.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Sep 18, 2015

I am not using TestCase and I have no experience with that. However, I would expect that tearDown() is called everytime, no matter if test was succesful.

I see setUp() and tearDown() as pair that set-ups and cleans environment, ie. creates and deletes resources, temporary files etc. However, from a practical point of view sometimes I don't want to delete temporary files when test fails because I want to analyse them, so method TestCase::hasFailed() can be useful.

Also I think that setUp() and tearDown() must not call any assertions, ie. an error in these methods should be considered as fatal test error. Sometimes it is needed to execute assertions on mocking tool after test - it should be done in method TestCase::assertPostConditions(), not in tearDown().

In PHP 7 we can catch PHP fatal errors and easily call tearDown, in PHP 5 we can simulate it via register_shutdown_function (with some limitations). We can implement it in Tester\Environment, via new callback $onFatalError or etc.

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 18, 2015

@milo oh I didn't notice :( pre-5.4 can be fixed, I've forgotten that closures didn't support $this in 5.3, but I have currently no idea why notice is not caught by php, but only php-cgi

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 18, 2015

@dg yes I simulated it with register_shutdown_function, but as @milo noticed now in Travis, it works only on "php-cgi" binary. I will do more investigation if it can be made working on php binary.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 18, 2015

@dg Agreed with most of what you said. Only exception is that tearDown should not do any asserts. It is common that tearDown holds fixture asserts. Things like test did not damaged anything, or mocking asserts. I do not think we need extra mechanism for that.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Sep 18, 2015

@richard-ejem there are a lot of cases (PHP limitations) when you cannot change exit code in register_shutdown_function https://github.com/nette/tester/tree/master/tests/Runner/edge.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Sep 18, 2015

@MartinMystikJonas I think that the separation of assertions from tearDown can solve a lot of problems. At least the dilemma what to call after failing test and what to not call.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 18, 2015

@dg Well I see advantages in that.

What about alternative approach using TestCase events as I proposed inPR #246 and #249 ? What do you think about that? We can move that asserts to this events.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Sep 18, 2015

@MartinMystikJonas As I wrote, I do not use TestCase, so I do not know, I can't imagine what it is useful for.

For Mockery::close() I need only one method which will be called after successful test, so one method with suitable name is for me better than a lot of events.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor

@MartinMystikJonas MartinMystikJonas commented Sep 18, 2015

This events are planned for more usages. I would like to implement something like Codeception modules with Nette tester and for that this events are necessary.

See this draft: https://forum.nette.org/en/24028-rfc-testcase-events-for-tester#p161290

@richard-ejem

This comment has been minimized.

Copy link
Contributor Author

@richard-ejem richard-ejem commented Sep 18, 2015

@dg so maybe if the exit code cannot be changed in register_shutdown_function in cli, what about registering custom error handler in TestCase?
I've finally built a version passing Travis in all cases - teardown is ran even on errors.

milo added a commit to milo/tester that referenced this issue Oct 9, 2015
milo added a commit to milo/tester that referenced this issue Oct 9, 2015
milo added a commit to milo/tester that referenced this issue Oct 9, 2015
richard-ejem added a commit to richard-ejem/tester that referenced this issue Oct 9, 2015
richard-ejem added a commit to richard-ejem/tester that referenced this issue Oct 9, 2015
milo added a commit to milo/tester that referenced this issue Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.