Include exception details on failure

[andrross]

Related Issues

Related to #15843

Check List

• Functionality includes testing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a116de2)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Loop Abort on First Failure Throwing AssertionError inside the loop (iterations 0–49) causes the loop to abort immediately on the first exception. The original fail() had the same behavior, but the intent of iterating 50 times suggests all responses should be checked. If any single response throws, the remaining 49 are never evaluated. Consider collecting failures and asserting after the loop, or at minimum be aware that only the first failure is reported. } catch (Exception t) { throw new AssertionError("search should not fail", t); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a116de2
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Include exception message in assertion error text Throwing AssertionError inside a catch (Exception t) block will not catch AssertionError itself (since it extends Error, not Exception), which is fine. However, if the intent is to propagate the original exception details while still failing the test, consider using fail("search should not fail: " + t.getMessage()) with the cause message, or rethrowing as a wrapped RuntimeException. More importantly, AssertionError thrown inside a loop will exit the loop immediately on the first failure, potentially hiding subsequent failures — this may or may not be the desired behavior. server/src/internalClusterTest/java/org/opensearch/search/SearchWeightedRoutingIT.java [464-465] } catch (Exception t) { - throw new AssertionError("search should not fail", t); + throw new AssertionError("search should not fail: " + t.getMessage(), t); } Suggestion importance[1-10]: 3 __ Why: The suggestion to include the exception message in the AssertionError text is a minor improvement for debugging, but the original cause t is already passed as the second argument to AssertionError, making the message redundant. The change from "search should not fail" to "search should not fail: " + t.getMessage() adds marginal value since the cause is already attached.	Low

---

Previous suggestions

Suggestions up to commit dcc6fc0

Category	Suggestion	Impact
General	Preserve exception details using test framework failure Throwing AssertionError inside a catch (Exception t) block will not be caught by the surrounding try block, but it also bypasses the test framework's normal failure reporting in some runners. Consider using fail("search should not fail: " + t.getMessage()) with the cause appended, or rethrowing as a RuntimeException wrapping the original cause, to ensure the full exception chain is visible in test reports while remaining consistent with the test framework. server/src/internalClusterTest/java/org/opensearch/search/SearchWeightedRoutingIT.java [463-465] } catch (Exception t) { - throw new AssertionError("search should not fail", t); + fail("search should not fail: " + t); } Suggestion importance[1-10]: 2 __ Why: The PR's change from fail() to throw new AssertionError("search should not fail", t) is actually an improvement because it preserves the original exception cause t in the stack trace, which the old fail("search should not fail") did not. The suggestion to revert to fail() with string concatenation is marginally less informative than using AssertionError with a cause parameter, and AssertionError is perfectly valid in JUnit test contexts.	Low

[github-actions]

❌ Gradle check result for dcc6fc0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit a116de2

[github-actions]

✅ Gradle check result for a116de2: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.28%. Comparing base (fb5d661) to head (a116de2).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20971      +/-   ##
============================================
+ Coverage     73.24%   73.28%   +0.04%     
+ Complexity    72510    72497      -13     
============================================
  Files          5819     5819              
  Lines        331373   331373              
  Branches      47882    47882              
============================================
+ Hits         242707   242849     +142     
+ Misses        69201    68994     -207     
- Partials      19465    19530      +65     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.