Use default cstack for tests and improve stackwalk test coverage #331
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes hardcoded cstack=fp configuration from test profiler commands and introduces parameterized testing to improve stack walk test coverage across different cstack modes (vm, fp, dwarf).
Changes:
- Removed explicit
cstack=fpfrom profiler command strings to use platform defaults - Extended JFR dump tests to run with multiple cstack modes via
@TestTemplateand@ValueSource - Migrated tests from
@RetryingTestto@RetryTestand introducedCStackAwareAbstractProfilerTestbase class
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| MemleakProfilerTest.java | Removed cstack=fp to use platform default |
| GCGenerationsTest.java | Removed cstack=fp to use platform default |
| WallclockDumpSmokeTest.java | Added parameterized testing for cstack modes and updated to new test infrastructure |
| ObjectSampleDumpSmokeTest.java | Added parameterized testing for cstack modes and updated to new test infrastructure |
| JfrDumpTest.java | Extended CStackAwareAbstractProfilerTest with cstack parameter support |
| CpuDumpSmokeTest.java | Added parameterized testing for cstack modes, removed hardcoded cstack=fp |
| ClassGCTest.java | Removed cstack=fp to use platform default |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void test() throws Exception { | ||
| @TestTemplate | ||
| @ValueSource(strings = {"vm", "fp", "dwarf"}) | ||
| public void test(@CStack String cstack) throws Exception { |
There was a problem hiding this comment.
The test method parameter cstack is annotated with @CStack but never used within the method body. If the cstack configuration is meant to influence test behavior, it should be passed to the test setup or profiler configuration.
| public void test() throws Exception { | ||
| @TestTemplate | ||
| @ValueSource(strings = {"vm", "fp", "dwarf"}) | ||
| public void test(@CStack String cstack) throws Exception { |
There was a problem hiding this comment.
The test method parameter cstack is annotated with @CStack but never used within the method body. If the cstack configuration is meant to influence test behavior, it should be passed to the test setup or profiler configuration.
| public void test() throws Exception { | ||
| @TestTemplate | ||
| @ValueSource(strings = {"vm", "fp", "dwarf"}) | ||
| public void test(@CStack String cstack) throws Exception { |
There was a problem hiding this comment.
The test method parameter cstack is annotated with @CStack but never used within the method body. If the cstack configuration is meant to influence test behavior, it should be passed to the test setup or profiler configuration.
zhengyu123
changed the title
| @Timeout(value = 60) | ||
| public void test() throws Exception { | ||
| @TestTemplate | ||
| @ValueSource(strings = {"vm", "fp", "dwarf"}) |
There was a problem hiding this comment.
I wonder - do we also want to include 'vmx' for completeness?
There was a problem hiding this comment.
I thought vm = vmx now, no?
There was a problem hiding this comment.
Well, kind of ... in our profiler we are not doing the 'features', so vmx is still valid specifier and translates to 'vm' + the extended feature ...
What does this PR do?:
Use platform default
cstackvalue for running tests. Also improve stack walk test coverage for some of tests.Motivation:
Improve test coverage.
Additional Notes:
NativeLibrariesTestis an exception, default value fails.How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!