Fixes bug in unit tests that meant exceptions were being "hidden" from callables and uses unchecked instead of checked exceptions #7
Conversation
…llables are not throwing exceptions as expected
…thrown as expected
… not forced to handle exceptions if they do not need to
…s to ignore exceptions when restoring original env var values
|
I have now added three more commits which implement the change I was originally intending to raise, this updates the execute method signatures for callable and statement so that a checked exception is no longer declared as being thrown. I have updated the method implementation to wrap the checked exception in an unchecked exception. The reason why I wanted this was that in the majority of the tests where I have used this library so far I am testing success cases rather than failure cases, and I personally it was a little annoying to have to add a throws Exception clause to all my tests. I really like the library so I thought I would have a go at updating it myself, and I found it very easy to do as it is very well written and tested, so thanks very much for sharing. |
michaelruocco
changed the title
| @@ -1,2 +1,4 @@ | |||
| .flattened-pom.xml | |||
| target | |||
| .idea | |||
| *.iml | |||
There was a problem hiding this comment.
This should be part of your personal global gitignore file. From the git docs:
Patterns which a user wants Git to ignore in all situations (e.g., backup or temporary files generated by the user’s editor of choice) generally go into a file specified by core.excludesFile in the user’s ~/.gitconfig.
There was a problem hiding this comment.
Ah okay nice, ill take a look at setting this up, thanks.
There was a problem hiding this comment.
It can be done that way, though it's probably a nicety to allow it in the repo too.
|
Thanks for the PR. I'm still undecided whether I want to have shorter stack traces in case of a failure or avoid the |
pom.xml
Outdated
| @@ -67,6 +67,7 @@ | |||
| <plugins> | |||
| <plugin> | |||
| <artifactId>maven-surefire-plugin</artifactId> | |||
| <version>3.0.0-M5</version> | |||
There was a problem hiding this comment.
I extracted this to a single commit 416ca75 because I like to have one commit per change so that its purpose can be explained in the commit message.
There was a problem hiding this comment.
sure no problem, I probably should have done that in the first place anyway.
|
Id have preferred not to have to re-throw Exception from each of my test but its not a big enough annoyance to diverge from using this version. Since the other elements have already been added to master now I have closed this down. |
|
@michaelruocco see whether https://github.com/webcompere/system-stubs is a reasonable alternative. You can avoid using the wrapper methods that throw and just plug in via a JUnit extension |
I was attempting to write a pull request to wrap the throwing of checked exceptions so that I don't have to add a
throws Exceptionclause to the end of all my tests. I will also raise a PR for this shortly, however in doing so I came across a small bug in the unit tests for the scenario where a callable throws an exception.The first commit in this PR shows how the bug can be recreated, I was expected the tests to receive an exception when I added the catchThrowable but they didn't. The second commit fixes the issue by removing the extra nesting levels for the lambda.
I also added an explicit version for the maven surefire plugin into the POM as I found when trying to run the tests this was failing for me since I didn't have the latest version.
Neither are big issues but since I came across them I thought I would share what I found.