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

Correct reporting of assertion errors and exceptions during code generation #1498

Merged
merged 14 commits into from
Jan 2, 2023

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Dec 6, 2022

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.

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.
Copy link
Collaborator

@petervdonovan petervdonovan left a 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.

org.lflang.tests/src/org/lflang/tests/TestBase.java Outdated Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Dec 7, 2022

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.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Dec 22, 2022

I reworked this completely. The exception TestError is now the one and only mechanism for reporting problems during test execution. This also allowed to improve the general design of LFTest and make all its internal state private.

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.

@lhstrh
Copy link
Member

lhstrh commented Dec 22, 2022

This will have some significant conflicts with what I'm doing in fed-gen. Specifically, I removed fileConfig from LFTest and made it accessible through the context. I'll put in a review when I can.

Copy link
Collaborator

@petervdonovan petervdonovan left a 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.

@petervdonovan
Copy link
Collaborator

Looks like yet another segfault in AsyncCallback. That shouldn't stop this from being merged since the issue was not created by this PR.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Dec 23, 2022

This will have some significant conflicts with what I'm doing in fed-gen. Specifically, I removed fileConfig from LFTest and made it accessible through the context. I'll put in a review when I can.

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.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jan 2, 2023

@lhstrh Is it Ok if I merge this? I can help to merge these changes into fed gen.

@cmnrd cmnrd merged commit 25b2207 into master Jan 2, 2023
@cmnrd cmnrd deleted the fix-test-reporting branch January 2, 2023 17:34
@lhstrh lhstrh changed the title Correctly report assertion errors and exceptions during code generation Correct reporting of assertion errors and exceptions during code generation Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants