-
-
Notifications
You must be signed in to change notification settings - Fork 448
run: Support system signal as a coverage report dump trigger. #1998
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Arkady Gilinsky <[email protected]>
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.
A few small changes. I'll adjust the English description once this merges.
coverage/cmdline.py
Outdated
@@ -227,7 +229,15 @@ class Opts: | |||
"", "--version", action="store_true", | |||
help="Display version information and exit.", | |||
) | |||
|
|||
dump_signal = optparse.make_option( | |||
'', '--dump_signal', action='store', metavar='DUMP_SIGNAL', |
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.
'', '--dump_signal', action='store', metavar='DUMP_SIGNAL', | |
'', '--dump-signal', action='store', metavar='DUMP_SIGNAL', |
Options use hyphens, not underscores.
coverage/cmdline.py
Outdated
@@ -227,7 +229,15 @@ class Opts: | |||
"", "--version", action="store_true", | |||
help="Display version information and exit.", | |||
) | |||
|
|||
dump_signal = optparse.make_option( |
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.
Overall, I think this should be called "save signal" instead of "dump signal" throughout.
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.
These make_option definitions are in rough alphabetical order, so this should be further up.
coverage/cmdline.py
Outdated
@@ -525,6 +536,7 @@ def get_prog_name(self) -> str: | |||
Opts.parallel_mode, | |||
Opts.source, | |||
Opts.timid, | |||
Opts.dump_signal, |
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.
Keep these items alphabetized.
Thanks for this! I want to have tests for this feature. |
* Set options in alphabetical order * Rename "dump-signal" to "save-signal" Signed-off-by: Arkady Gilinsky <[email protected]>
Hi, Thanks. |
Thanks, what are your thoughts on a test? Let me know how far you want to take this, I can take it over when you want. |
I think that test should run some script with 2 stages.
But for the above I need to run script in background and to communicate with the running script dynamically (to monitor it's output). |
The test would have to be something like that, but I don't want it to take 10 seconds. I think it would be enough to have two cases in the test: in both, start an infinite loop process with |
BTW, we also need to do the right thing on Windows. I think it's best to add "not on Windows" to the option help, and print an error if it's used on Windows. The test should check this also. |
Do you have an example how to initiate infinite loop process in existing test infrastructure or I should use Subprocess module in the test ? |
Signed-off-by: Arkady Gilinsky <[email protected]>
I do not see much more purpose on the second test (with only KILL signal), but as you wish ... ;) |
Signed-off-by: Arkady Gilinsky <[email protected]>
Did you see this pull request? ark-g#1 It has some other changes for the code in cmdline.py. I wanted the test without the signal to show that the signal is needed to get the data file. |
I think that the first test is also demonstrating this fact, since the final coverage report doesn't include the line that is sending KILL signal, but does include the line with USR1 signal. Do you think that I can merge this PR ? |
I want you to take the changes I gave you in the pull request in your repo. |
Implement a feature that allows users to generate a coverage report by sending a signal. This is particularly useful for obtaining coverage reports from continuously running Python scripts, such as server implementations or monitoring utilities.
Currently supported only two signals SIGUSR1 and SIGUSR2.