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

Add unit tests for ExceptionExtensions, #1112 #1120

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

paulirwin
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Add unit tests for ExceptionExtensions.

Fixes #1112

Description

Adds unit tests for Lucene.Net.Util.ExceptionExtensions around handling suppressed extensions.

@paulirwin paulirwin added the notes:improvement An enhancement to an existing feature label Jan 22, 2025
Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

During the review I noticed that addSuppressed(), getSuppressed(), and printStackTrace() are all synchronized. This makes sense because calling addSuppressed() could change the result of the other two.

https://github.com/EricChows/JDK-1.8-sourcecode/blob/master/java/lang/Throwable.java#L1041-L1080

I can't think of a use case where we actually need thread safety here. We are dealing with exceptions in catch blocks where it is impossible for 2 threads to interact with a single exception unless parallel execution is started inside of the catch block (or after if the exception is set to a variable).

But, if you wish to future-proof the implementation or know of some place where this will cause a problem, I have no objections to adding synchronization (and tests).

@paulirwin
Copy link
Contributor Author

I'll skip synchronization for now, it can always be added later as a fix without breaking the contract if there are any issues with it. I doubt there will be, though.

@paulirwin paulirwin merged commit 5007427 into apache:master Jan 22, 2025
8 checks passed
@paulirwin paulirwin deleted the issue/1112 branch January 22, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:improvement An enhancement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support unit tests for ExceptionExtensions
2 participants