-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Use assertThrows in classes ActorCreationTest and OutputStreamSinkTest #30165
Conversation
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK ΤO ΤESΤ' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
OK TO TEST |
Test FAILed. |
Test FAILed. |
Hm... It looks like some infrastructure error And some strage test |
Yeah, was something temporary yesterday, I'll trigger a rebuild. |
PLS BUILD |
Test FAILed. |
Test FAILed. |
I don't understand indeed. The test |
Unless you can figure it out and make the test pass, yes revert please. |
Test PASSed. |
@johanandren review it, please |
I found why that other test was not failing. That's because the original one was also not failing: try {
Props.create(
Actor.class,
new Creator<Actor>() {
@Override
public Actor create() throws Exception {
return null;
}
});
assert false;
} catch (IllegalArgumentException e) {
assertEquals(
"cannot use non-static local Creator to create actors; make it static (e.g. local to a static method) or top-level",
e.getMessage());
} That won't fail, because it is using java assertions. Assertions aren't enabled by default. So, the Then, the Replace the |
@Captain1653, do you want to take a stab on it and try to fix that test? By fixing, I mean, ensure that it will indeed fail the call and throw an |
@octonato Thank you for your explanation. I didn't guess))). Yes, I can try to do it. It looks interesting. Should it be some different issue and different PR? For me - yes, because it's the different problem (not refactor). What do you think? P.S. I tried to search places with "assert " in source code. It was used many times. Is it worth to fix it in the different issue and different PR? (replace on assertEquals or similar) Kind regards |
Yes, you are right. I think we should fix it on a different PR. Let's not mix things. |
Thare are many lines look like that:
try ... catch
in tests. It can be replaced on assertThrowsKind regards