fix: Prevent crash when test fails on a different thread #307
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where exceptions thrown from different threads would cause an InternalError during test execution. The fix introduces a fallback mechanism in the error frame detection logic to handle cases where the test class is not in the exception's stack trace.
- Added a helper method
isFrameworkClass()to identify known framework/JDK classes - Modified
findErrorFrame()to use a fallback heuristic when the test class isn't found in the stack trace - Added test case and verification for off-thread exception handling
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| agent/src/main/java/com/appland/appmap/process/hooks/test/TestSupport.java | Implements fallback logic in findErrorFrame() to handle exceptions from different threads by tracking non-framework stack frames |
| agent/test/test-frameworks/src/test/java/org/springframework/samples/petclinic/JunitTests.java | Adds ExecutorRunner helper class and offThreadExceptionTest() to reproduce off-thread exception scenario |
| agent/test/test-frameworks/frameworks.bats | Adds test to verify no InternalError occurs with off-thread exceptions, and updates line number in existing assertion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agent/test/test-frameworks/src/test/java/org/springframework/samples/petclinic/JunitTests.java
Outdated
Show resolved
Hide resolved
agent/src/main/java/com/appland/appmap/process/hooks/test/TestSupport.java
Outdated
Show resolved
Hide resolved
agent/src/main/java/com/appland/appmap/process/hooks/test/TestSupport.java
Outdated
Show resolved
Hide resolved
agent/test/test-frameworks/src/test/java/org/springframework/samples/petclinic/JunitTests.java
Outdated
Show resolved
Hide resolved
dividedmind
force-pushed
the
fix/cross-thread-exception-in-test-failure
branch
2 times, most recently
from
cf62ae0 to
f7b6d04
Compare
hleb-rubanau
left a comment
hleb-rubanau
left a comment
There was a problem hiding this comment.
@dividedmind I have nothing to say regarding the code (which is clean and logical as usual), but CI has multiple failures currently -- at least some of them happen due to path expectations mismatch, not sure about others.
https://github.com/getappmap/appmap-java/actions/runs/18916960063/job/54107389605
Can you please take a look?
dividedmind
force-pushed
the
fix/cross-thread-exception-in-test-failure
branch
from
f7b6d04 to
2240be0
Compare
To speed up CI, download a tarball of a specific commit hash instead of performing a full git clone. This is particularly useful for pinning spring-petclinic to a version compatible with Java 17. After extracting the tarball, a new git repository is initialized to simulate a checkout (which the tests expect). Refactor the script to handle different Java versions and use the appropriate petclinic version for each. This commit also updates the appmap-labels.yml file to correctly identify the logging classes used in newer versions of the project.
When a test fails due to an exception thrown on a different thread, the test class itself may not appear in the exceptions stack trace. The agent was previously unable to handle this scenario, crashing with a `java.lang.InternalError` because it could not find a stack frame matching the test class. This commit introduces a more robust heuristic to find the most likely source of the failure. If an exact match for the test class is not found, the agent now falls back to the stack frame with the longest common package prefix. This prevents the crash and correctly identifies the failure location in a framework-agnostic way. A regression test has been added to simulate this cross-thread exception scenario and verify the fix.
dividedmind
force-pushed
the
fix/cross-thread-exception-in-test-failure
branch
from
2240be0 to
d0d19c8
Compare
Fixed! I also changed the code a bit — I used a different heuristic which I think is more reliable. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
agent/src/main/java/com/appland/appmap/process/hooks/test/TestSupport.java
Show resolved
Hide resolved
|
🎉 This PR is included in version 1.28.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
When a test fails due to an exception thrown on a different thread, the test class itself may not appear in the exception's stack trace. The agent was previously unable to handle this scenario, crashing with a
java.lang.InternalErrorbecause it could not find a stack frame matching the test class.This commit introduces a more robust heuristic to find the most likely source of the failure. If an exact match for the test class is not found, the agent now falls back to the stack frame with the longest common package prefix. This prevents the crash and correctly identifies the failure location in a framework-agnostic way.
A regression test has been added to simulate this cross-thread exception scenario and verify the fix.
Fixes #306