-
Notifications
You must be signed in to change notification settings - Fork 63
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
Correct reporting of assertion errors and exceptions during code generation #1498
Conversation
The TestBase class masked all assertion error as the AssertionError throwable was used as a marker for reporting a test failure and aborting early without additional error reporting. This change instead introduces a custom exception type and make sure that assertion errors and other exceptions are properly executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a minor comment, but mostly this looks good to me.
This looks good to me too, but the error in ccp-tests should be looked at. Unrelated to the changes in the PR, I'm sure, but worrisome nonetheless. |
I reworked this completely. The exception Thanks @oowekyala for your review comments! I was a bit annoyed at first because it means more work, but I think the new design is much more robust and there should be an error message printed regardless of why the test failed. |
This will have some significant conflicts with what I'm doing in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a minor comment. If I was mistaken or if it gets fixed, then I think this is good to merge.
Looks like yet another segfault in |
I am happy to help resolve any merge conflicts. I feel like this refactoring was needed and it should actually make it easier to implement changes in the future. |
@lhstrh Is it Ok if I merge this? I can help to merge these changes into fed gen. |
While working on #1441, I noticed that assertion errors during code generation are not correctly reported by our test framework. This is because the TestBase used AssertionError to indicate that a test should be aborted without additional reporting. This change instead introduces a custom exception type for aborting tests and makes sure that assertion errors and other exceptions are properly reported.