fix(java-runtime): ErrorStreamSink crashes with NullPointerException
#4803
Conversation
| runtime.getClient().createObject("Object", Collections.emptyList(), Collections.emptyList(), | ||
| Collections.emptyList()); | ||
| runtime.terminate(); | ||
| Assertions.assertFalse(err.toString().contains( |
There was a problem hiding this comment.
Better ideas (that don't require a major refactor) are welcome.
There was a problem hiding this comment.
Could we instead test for the explicit error output we expect? That should be known and stable for the test, no?
assertEquals(err.toString(), "whatever the expected output is");There was a problem hiding this comment.
the throwable object is also in that message and i'd prefer not to assert on stack traces.
I think it asserts on enough of the message to make it unique to this scenario, no?
|
Thank you! I would assert that the output is empty, rather than it doesn't contain this specific error. But, thank you for adding the test! |
|
Feel free to raise this against my branch instead- that way we both get the commit credits (I noticed mine are missing from this one for some reason) |
|
@TimothyJones how about just copy-paste the code for the test I added and submit a new PR with it? That would also work, than I can approve it and merge :) |
Co-authored-by: TimothyJones <[email protected]>
iliapolo
force-pushed
the
epolon/error-steam-sink-crash
branch
from
6d1cfd0 to
b9c3a10
Compare
|
@TimothyJones I added you as a co-author to the commit. Thanks again for your work on this issue. |
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
|
Thank you! Co-author is great. Thanks for merging! |
All credit for the detection and fix goes to @TimothyJones and his PR. This just adds a unit test on top of it.
Thanks @TimothyJones !
Fixes #4801
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.