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

Exceptions from setUp and tearDown are not subject for @throws annotation #238

Closed

Conversation

@MartinMystikJonas
Copy link
Contributor

MartinMystikJonas commented Aug 5, 2015

When Exception is thrown from setUp or TearDown it was processed same as when it is thrown from test itself. Combined with @throws annotation this can lead to false positive tests. For example when exception in expected from test but was not thrown due to error byt this error in test also caused throwing od exception from tearDown it passes.

Also covers case when exception is thrown both from test and tearDown.

@MartinMystikJonas MartinMystikJonas force-pushed the MartinMystikJonas:test-run-exception branch 5 times, most recently from fbf3c34 to b5e4962 Aug 5, 2015
@MartinMystikJonas MartinMystikJonas changed the title Exceptions from setUp amd tearDown are not subject for @throws annotation Exceptions from setUp and tearDown are not subject for @throws annotation Aug 5, 2015
@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 5, 2015

Related #52.

What about simplier TestCaseException with message like setUp() failed, $e as previous and enhace condition here for TestCaseException?

@MartinMystikJonas MartinMystikJonas force-pushed the MartinMystikJonas:test-run-exception branch from b5e4962 to 8224f43 Aug 6, 2015
@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 6, 2015

#52 is my original solution of similar problem, but it turns out it is not sufficient with combination with @throws annotation

I added condition for TestCaseException and made TestRunException extends TestCaseException. But have it as separate class is important for case when exception both from test itslef and tearDown is thrown - in that case it is important to see both exceptions when you trying to figure what went wrong. Also it is important for this exceptions to carry information about what test method caused this failure otherwise it would be very difficult to tract source of error - solved by option to use appendMessage to append " in testMethod()".

See new version.

@MartinMystikJonas MartinMystikJonas force-pushed the MartinMystikJonas:test-run-exception branch from 8224f43 to 5132deb Aug 6, 2015
@milo milo added the bug label Aug 6, 2015
@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 6, 2015

when exception both from test itslef and tearDown is thrown - in that case it is important to see both exceptions

Why need to see both? Because of exact call order (setUp() -> test() -> tearDown()), you just need to see the first which happend. After that, test exits. You fix it and when thrown in next phase, you fix the next one.

Method name appending is important, but separate it into next commit please.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 6, 2015

You need to see both in case first exception (from test) is expected (specified by @throws) but second exception from tearDown is not. For example test correctly throws exception but then Mockery checks in tearDown fails. Without seeing both exceptions you will only know that test correctly throws exception but not what failed then in tearDown. For same test alternative scenario is that test throws unexpected exception and also Mockery checks failed. In this case you want to see exception from test.

@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 7, 2015

Still don't understand :o) What's wrong on this solution?

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 7, 2015

It can pass when should not. If test throw SomeException (which is expected) and tearDown throws MockeryException then MockeryException will be ignored. So if SomeException is expected by @throws annotation test will pass every time SomeException is thrown even if some mockery expectations is not met.

This is exact error guide us to this change.

@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 7, 2015

I see now. What about such fix?

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 7, 2015

I'm not sure it will work. But it could. Can you test it with tests I provided?

But I do not like new state introduced for TestCAse. I think my solution with specialized exception is cleaner.

@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 7, 2015

It will work. I don't like the exception message mangling in this PR and I prefer the more language natural way.

@dg Don't you have a next idea how to solve this?

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 7, 2015

Well what about solution where TestCaseExceotion could contain two original expections.

throw new TestCaseException("tearDownFailed", 0, $testException, $tearDownException)

And then process this similary to your solution but take tearDownException from exception thrown from runTest instead of TestCase hidden state? (I can prepare PR tomorrow)

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 8, 2015

Your solution will not work in two cases:

Test annotated @throws MyException
It will throw UnexpectedException, then tearDown fails.
Result: you see that 'TestCaseException(tearDown failed)' is not expected 'MyException', but information about UnexpectedException which is main cause is lost.

Test annotated @throws MyException
Test by error dont throw anything, then tearDown fails.
Result: You see that 'TestCaseException' is not expected 'MyException', but information that test don't throw anything is lost.

I tried different solutions for almost two hours but I did not find any which can cover all use cases. I think we have only three options:
A) Allow information loss
B) My solution with combined Exceptions to prevent loss of any information
C) Refactor TestCase and move this:

$e = Assert::error(function () use ($tmp, $method, $args) {
  $tmp->runTest($method->getName(), $args);
}, $throws[0], $throws[1]);

inside runTest method so it wraps only test method itself and not setUp and tearDown to prevent checking tearDown exceptions agains expectations defined by @throws for test method.

In fact option B will be probably better because it also prevent mixing errors from test with errors from setUp and tearDown.

But anything is better than current state with false passing tests.

@MartinMystikJonas MartinMystikJonas force-pushed the MartinMystikJonas:test-run-exception branch from 5132deb to 8bfeab9 Aug 8, 2015
@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 8, 2015

I was able to create solution which covers most use cases correctly. It is quite complicated code but works as expected. See tests.

Possible misbehaviour is when excepting E_ and same error was generated in tearDown and expecting \Exception with message where TestCaseException is also exception and tehrefore message from TestCaseException will be checked against expected message.

But this is rare special cases and I think current solutiion is sufficient for most use cases. Maybe merge this PR to prevent commont false positives and think about solution for this rare cases. But I am afraid iw will not be possible without refactoring of testCase internal structure.

@MartinMystikJonas MartinMystikJonas force-pushed the MartinMystikJonas:test-run-exception branch from 8bfeab9 to 4ab8c21 Aug 8, 2015
@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 10, 2015

@milo: How to you want to continue on this? I would propose at least do quick fix which will prevent false passing tests ASAP. Then we can discuss what to do next.

@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 10, 2015

It is nasty bug, but it is present ages here so it will wait. Right now, I'am trying some ways...

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 10, 2015

From my POW best solution will be to apply @thorws annotation checks only to test method itself inside runTest. Question is how to pass needed parameters there. What do you think about third parameter to runTest?

public function runTest($name, array $args = array(), $throws = NULL) {
  //...
  $tmp = $this;
  $e = Assert::error(function () use ($tmp, $name, $args) {
    call_user_func_array(array($this, $name), $args);
  }, $throws[0], $throws[1]);
  //...
}

Second option is to parse throws from doc comment inside runTest. This could be best option from my POW because i will expect this behaviour when I call runTest on method with @throws.

I can prepare PR with preferred version.

milo added a commit to milo/tester that referenced this pull request Aug 10, 2015
…loses nette#238]

BC break: when TestCase::runTest() overloaded and do not call the parent, tests will start to fail because an expected exception will not be catched.
@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 10, 2015

@MartinMystikJonas What about this solution? milo@bb769e6

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 10, 2015

Looks good.

My only objection is using testRunner as TestCase hidden state. It's does not look clean to me. But if we do not want introduce new parameter in runTest it is only way so I think it is ok.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 10, 2015

Found one problem: When test throws exception tearDown is not called. Common approach is to call tearDown regardless is test suceed. It is because tearDown can contain clean up rutines needed to other test work properly. It prevents cacsade failure triggered by one failing test.

But u can use original approach to throwing exceptions:

    public function runTest($name, array $args = array())
    {
        $this->setUp();

        try {
            if ($this->testRunner) {
                call_user_func($this->testRunner, array($this, $name), $args);
            } else {
                call_user_func_array(array($this, $name), $args);
            }
        } catch (\Exception $e) {
        }

        try {
          $this->tearDown();
        } catch (\Exception $tearDownEx) {
        }

        if (isset($e)) {
            throw $e;
        }

        if (isset($tearDownEx)) {
            throw $tearDownEx;
        }
    }
@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 10, 2015

Good point. Is's obviously not tested :o) I'll append it.

milo added a commit to milo/tester that referenced this pull request Aug 10, 2015
…loses nette#238] (thanks to @MartinMystikJonas)

BC break: when TestCase::runTest() overloaded and do not call the parent, tests will start to fail because an expected exception will not be catched.
@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 10, 2015

Final solution looks good. 👍 But I do not think it is BC break. Method runTest does not check expected exception in previous version too. Or am I missing something?

@milo

This comment has been minimized.

Copy link
Member

milo commented Aug 10, 2015

There is a small probability that someone implemented own runTest() and didn't call parent. The expected Exception has been caught in runMethod() but now, it will not be catched and test will fail.

@MartinMystikJonas

This comment has been minimized.

Copy link
Contributor Author

MartinMystikJonas commented Aug 10, 2015

I see.

milo added a commit to milo/tester that referenced this pull request Aug 10, 2015
milo added a commit to milo/tester that referenced this pull request Aug 12, 2015
… setUp() nor tearDown() (BC break) [Closes nette#238]

BC break: runTest() 2nd parameter defaults is NULL, empty array bypasses dataprovider
BC break: runTest() now handles @throws annotation, expected exception is catched
@milo milo closed this in 710c4b5 Aug 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.