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

Why shouldThrow<> {} always pass this check? #479

Closed
bahka opened this issue Nov 21, 2018 · 2 comments · Fixed by #484
Closed

Why shouldThrow<> {} always pass this check? #479

bahka opened this issue Nov 21, 2018 · 2 comments · Fixed by #484
Assignees
Labels
bug 🐛 Issues that report a problem or error in the code.
Milestone

Comments

@bahka
Copy link

bahka commented Nov 21, 2018

In documentation you said:

Exceptions
To assert that a given block of code throws an exception, one can use the shouldThrow function. Eg,

shouldThrow<IllegalAccessException> {
  // code in here that you expect to throw an IllegalAccessException
}

But in reality it green always

Context: just for fun - green
Scenario: throw exception - green
Scenario: no exception - green

        context("just for fun") {
            // pass ??? why  
            //  even I set wrong Exception type -   there should be AssertionFailedError
            it("throw exception") {
                shouldThrow<java.lang.AssertionError> { 1 shouldBe 2 }
            }
            // pass ??? why
            it("no exception") {
                shouldThrow<java.lang.AssertionError> { 1 shouldBe 1 }
            }
        }

====
If I change shouldThrow<java.lang.AssertionError> to shouldThrowExactly<java.lang.AssertionError> it works well now:

 context("just for fun") {
            //  pass
            it("throw exception") { 
                shouldThrowExactly<org.opentest4j.AssertionFailedError> { 1 shouldBe 2 }
            }
            // failed
            it("no exception") {
                shouldThrowExactly<org.opentest4j.AssertionFailedError> { 1 shouldBe 1 }
            }
        }
@LeoColman
Copy link
Member

Thanks for your report!

For the first test, the reason

it("throw exception") {
    shouldThrow<java.lang.AssertionError> { 1 shouldBe 2 }
}

passes is because AssertionFailedError is instance of AssertionError. So any AssertionError is OK for shouldThrow. If you create an Exception such as
class MyException : AssertionError(), it will pass shouldThrow<AssertionError> as well.

When you change it to shouldThrowExactly in the second scenario, you're verifying correctly that it's exactly the same exception you wanted.

However, on your first example:

    shouldThrow<java.lang.AssertionError> { 1 shouldBe 1 }
}

And your second example

it("no exception") {
                shouldThrowExactly<org.opentest4j.AssertionFailedError> { 1 shouldBe 1 }
            }

It shouldn't pass, and you stand correct there.

However, there's one small catch there: This bug will only happen when you're doing it with an AssertionError of some sort, and no other exceptions.

I'll look into it further, but for now this information may be relevant.

@LeoColman LeoColman self-assigned this Nov 21, 2018
@LeoColman LeoColman added the bug 🐛 Issues that report a problem or error in the code. label Nov 21, 2018
@LeoColman
Copy link
Member

I found the point that is bugged. Working on a solution now.

@sksamuel sksamuel mentioned this issue Nov 28, 2018
17 tasks
LeoColman added a commit that referenced this issue Dec 3, 2018
* Refactors Throwable Handling to it's own package, fixing bugs

This commit moves ThrowableHandling to their own package, as it's a pattern KT is following for a while. While moving the classes and updating the package, this commit also adds documentations to all use cases, and add some extra cases that `shouldThrowUnit`, such as `shouldThrowAnyUnit`.

This commit deprecates usage, that should be removed from the library in a future. For now, code will still compile.

By changing the order in which verifications are made, this commit also fixes #479, and adds a test for every possible scenario, stabilizing and reviewing these methods

* Moves Throwable Handling to io.kotlintest package

As discussed in the pull request, there won't be any problem to keep these functions in the `io.kotlintest` package, as they're not too many. This also will avoid issues with the user, as they won't have to change anything for the bugs to be fixed and the code to still work.

* Changes `shouldFail` to work with AssertionError only and improves it's documentation

* Reworded documentation to say that assignments are statements

* Changes Failures implementation to delegate to shouldThrow<AssertionError>

* Fixes broken tests

Some test scenarios broke. They were depending on a bug to work (reported in #479), and they weren't testing correctly. When the bug was gone, the tests started to fail, but fixing their test forces them to work again.
@sksamuel sksamuel added this to the 3.2 milestone Dec 3, 2018
sksamuel pushed a commit that referenced this issue Dec 4, 2018
* Refactors Throwable Handling to it's own package, fixing bugs

This commit moves ThrowableHandling to their own package, as it's a pattern KT is following for a while. While moving the classes and updating the package, this commit also adds documentations to all use cases, and add some extra cases that `shouldThrowUnit`, such as `shouldThrowAnyUnit`.

This commit deprecates usage, that should be removed from the library in a future. For now, code will still compile.

By changing the order in which verifications are made, this commit also fixes #479, and adds a test for every possible scenario, stabilizing and reviewing these methods

* Moves Throwable Handling to io.kotlintest package

As discussed in the pull request, there won't be any problem to keep these functions in the `io.kotlintest` package, as they're not too many. This also will avoid issues with the user, as they won't have to change anything for the bugs to be fixed and the code to still work.

* Changes `shouldFail` to work with AssertionError only and improves it's documentation

* Reworded documentation to say that assignments are statements

* Changes Failures implementation to delegate to shouldThrow<AssertionError>

* Fixes broken tests

Some test scenarios broke. They were depending on a bug to work (reported in #479), and they weren't testing correctly. When the bug was gone, the tests started to fail, but fixing their test forces them to work again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Issues that report a problem or error in the code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants