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

Describe actual Throwables when ThrowableAssert fails #1355

Open
3 of 12 tasks
jakubzytka opened this issue Nov 8, 2018 · 23 comments · May be fixed by #2583, #2627 or #3392
Open
3 of 12 tasks

Describe actual Throwables when ThrowableAssert fails #1355

jakubzytka opened this issue Nov 8, 2018 · 23 comments · May be fixed by #2583, #2627 or #3392
Assignees
Labels
status: ideal for contribution An issue that a contributor can help us with type: improvement A general improvement

Comments

@jakubzytka
Copy link

jakubzytka commented Nov 8, 2018

Summary

When some Throwable-related assertion fails the actual Throwable that failed the check is not described beyond what actually caused assertion failure (e.g. message mismatch). In non-trivial code when unexpected Exceptions are thrown this makes debugging inconvenient.

Dumping more comprehensive (class, message, stack, perhaps even recursive info about causes) info about the actual Throwable could ease assertion failure investigation.

Relevant message factories (e.g. ShouldHaveMessage) already have access to actual Throwable.

It may be tempting to use util.Throwables.ERROR_DESCRIPTION_EXTRACTOR to describe throwables, but a more powerful mechanism would be appreciated.
Providing precise information in case of assertion failure doesn't seem to be overkill (unlike as in #864)

Error message to improve

If you are interested in contributing to this issue, select one error message, it will be assigned to you.

@joel-costigliola
Copy link
Member

joel-costigliola commented Nov 8, 2018

do you mean printing the stack trace at the end of the error message ?

can you give an example of what you would ideally have ?

@jakubzytka
Copy link
Author

Example of my ideal message:

Expecting Throwable message:
<"expected exception message">
but was:
<"some other unexpected exception">

Throwable that failed the check:
Exception in thread "main" java.lang.RuntimeException: some other unexpected exception
	at Test.functionThatIsSupposedToThrowExpectedException(Test.java:119)
	at Test.main(Test.java:69)
Caused by: java.lang.RuntimeException: unexpected exception in recurseToGenerateSomeStack
	at Test.recurseToGenerateSomeStack(Test.java:133)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.recurseToGenerateSomeStack(Test.java:138)
	at Test.functionThatIsSupposedToThrowExpectedException(Test.java:115)
	... 1 more
Caused by: java.lang.RuntimeException: the root cause of test failure
	at Test.f(Test.java:169)
	at Test.e(Test.java:164)
	at Test.d(Test.java:159)
	at Test.c(Test.java:154)
	at Test.b(Test.java:149)
	at Test.a(Test.java:144)
	at Test.recurseToGenerateSomeStack(Test.java:129)
	... 13 more

Such message allows to immediately learn the real cause of test failure.

@joel-costigliola joel-costigliola added this to the 3.13 milestone Mar 3, 2019
@joel-costigliola
Copy link
Member

Fair enough, @jakubzytka would you like to contribute it?

@joel-costigliola
Copy link
Member

I have done it for the hasMessage assertion, it should also done for other throwable assertions.

@steveorourke
Copy link
Contributor

Submitted #1630 to do the same for hasMessageMatching and hasMessageFindingMatch assertions.

@benoitdupont
Copy link

Hello, is this issue still open ? I did try the various method asserting the messages in AbstractThrowableAssert but didn't find any case where the stacktrace is not displayed. Or can you point the specific methods to fix ?

@jakubzytka
Copy link
Author

Throwables.assertHasMessageContaining doesn't describe the actual throwable (neither do some other functions that care about the message)

@benoitdupont
Copy link

Hello, I did a first pull request to fix the hasMessageContaining and hasMessageContainingAll assertions, let me know if this is correct.

Next steps is to investigate and fix which methods can still be improved, here is a list to be refined of potential classes to be adapted:

// done in upper commits
ShouldHaveMessage 
ShouldHaveMessageFindingMatchRegex
ShouldHaveMessageMatchingRegex
ShouldHaveRootCause

// still need to adapt the stackTrace case
ShouldContainCharSequence.shouldContain 

// to be adapted (maybe doesn't apply)
ShouldEndWith
ShouldHaveCause
ShouldHaveCauseExactlyInstance
ShouldHaveCauseInstance
ShouldHaveCauseReference
ShouldHaveNoCause
ShouldHaveNoSuppressedExceptions
ShouldHaveRootCauseExactlyInstance
ShouldHaveRootCauseInstance
ShouldHaveSuppressedException
ShouldNotContainCharSequence
ShouldStartWith

@RGalways17
Copy link
Contributor

Hi, I've created a PR fixing the hasCauseInstanceOf and hasCauseExactlyInstanceOf. Please let me know if there is any problem:-).

@weiyilei
Copy link

Is anybody working on this? If not, can I?

@joel-costigliola
Copy link
Member

thanks @weiyilei, I have assigned you ShouldHaveCauseReference, you can look at ShouldHaveCauseInstance for reference.

@weiyilei
Copy link

thanks @weiyilei, I have assigned you ShouldHaveCauseReference, you can look at ShouldHaveCauseInstance for reference.

thank u bro, but I am really confused about what kind of reference will be required in this issue, can u give me an example plz

@weiyilei
Copy link

thanks @weiyilei, I have assigned you ShouldHaveCauseReference, you can look at ShouldHaveCauseInstance for reference.

Hi bro, I still have no idea about what does the cause reference mean, as I know A.getcause() can get the throwable which lead to A, so what's its reference. :(
Besides, I want to know whether this issue is still open and remains any job to be finished, thanks.
Add more basic assertions for Local(Date|Time) #2541

@weiyilei
Copy link

thanks @weiyilei, I have assigned you ShouldHaveCauseReference, you can look at ShouldHaveCauseInstance for reference.

should I use getStackTrace() or getSuppressed(), or import org.openjdk.jol.vm.VM and use VM.current().addressof()

@joel-costigliola
Copy link
Member

ShouldHaveCauseInstance is the example

@weiyilei
Copy link

ShouldHaveCauseInstance is the example

Hi, I am wondering if the content of ShouldHaveCauseReference_create_Test.java should be modified, or I should just fill the methods of ShouldHaveCauseReference according to the test content

@joel-costigliola
Copy link
Member

I'm not sure I understand the question, if you modify ShouldHaveCauseReference you have to write the tests that proves your changes are working and thus you would need to add test cases in ShouldHaveCauseReference_create_Test

@joel-costigliola
Copy link
Member

joel-costigliola commented Apr 19, 2022

@weiyilei if you still don't understand what I'm saying it means you are not yet ready to contribute and you need a bit more experience before doing so.

The AssertJ team is working on its spare time maintaining this project, it's not that we don't want to help junior devs to contribute, it's just that we don't have the time to do so.

This was referenced Apr 20, 2022
@weiyilei
Copy link

I'm not sure I understand the question, if you modify ShouldHaveCauseReference you have to write the tests that proves your changes are working and thus you would need to add test cases in ShouldHaveCauseReference_create_Test

Hi bro, I have tried my best to understand ur requirements and finished my codes following the example of CauseInstance, I have a pull request and let me know if there is something I need to improve. thanks

@HDreamer2
Copy link
Contributor

@joel-costigliola Hi, I'm interested in this issue. Is there any available task for me to do?

@Ds2994
Copy link
Contributor

Ds2994 commented Dec 9, 2022

Hi @joel-costigliola I created a PR to update the ShouldHaveCause with Stacktrace. Please let me know if there is a problem.

@Ds2994
Copy link
Contributor

Ds2994 commented Jan 10, 2023

Hi @joel-costigliola, Thanks for reviewing my earlier PR. I have created a new one for ShouldHaveRootCauseInstance and ShouldHaveRootCauseExactlyInstance. Please let me know if there is any problem with the same.

@shaikhu
Copy link
Contributor

shaikhu commented Mar 22, 2024

Hi @joel-costigliola , can ShouldHaveCauseExactlyInstance be marked as done now? Also I have a PR up for ShouldHaveCauseInstance.

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