-
Notifications
You must be signed in to change notification settings - Fork 11
Add a test for crash-recovery in the 'vm' stackwalker. #216
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
Conversation
|
🔧 Report generated by pr-comment-scanbuild |
|
|
||
| #ifdef DEBUG | ||
| #include <signal.h> | ||
| static const char* force_stackwalk_crash_env = getenv("DDPROF_FORCE_STACKWALK_CRASH"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple but efficient 👍
|
|
||
| public abstract class AbstractProcessProfilerTest { | ||
| protected final boolean launch(String target, List<String> jvmArgs, String commands, Function<String, Boolean> onStdoutLine, Function<String, Boolean> onStderrLine) throws Exception { | ||
| public static final class LaunchResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see we are investing in controlled behaviour tests.
If we want to extend these, we could discuss what would be a good structure.
Nothing blocking though it is hard to see the structure. I would expect something like:
TestProcessController controller = new TestProcessController(WorkloadType.CPU_INTENSIVE);
controller.launch();
controller.waitForReady();
controller.sendStartSignal();
controller.waitForWorkingOutput();
controller.assertProfilerDidX();
controller.shutdown();
I think we basically have different parts
- controller
- assertions
- workload
- protocol
r1viollet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bcb1591 to
ac3cd0d
Compare
What does this PR do?:
This adds a smoke test for the crash-recovery mechanism used in the 'vm' stackwalker
Motivation:
A followup for bugfix #214
How to test the change?:
The test runs automatically
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!