-
Notifications
You must be signed in to change notification settings - Fork 18
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
ENH: Automated Bug Reporting #92
Conversation
Instead of repeating the code to find a handler of a certain name, a generic function was created to find a handler of any name. This is then used to find the base log file name and search the directory for any other log messages that may have been created.
Codecov Report
@@ Coverage Diff @@
## master #92 +/- ##
==========================================
- Coverage 98.75% 97.62% -1.13%
==========================================
Files 14 15 +1
Lines 560 674 +114
==========================================
+ Hits 553 658 +105
- Misses 7 16 +9
Continue to review full report at Codecov.
|
After sleeping on it. I think I'll switch this to use an |
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.
I like this implementation, see comments for suggestions for changes. Consider trying to bump up the coverage too, bug.py
is the lowest-coverage file in the module by a fair bit.
bin/post-issues
Outdated
|
||
|
||
description = """\ | ||
Post all of the stored issue to https://github.com/pcdshub/Bug-Reports |
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.
typo, issue
should be issues
bin/post-issues
Outdated
pw = args.password or getpass.getpass() | ||
# Run post_to_github on every stored issue | ||
for issue in os.listdir(BUG_REPORT_PATH): | ||
if issue.endswith('.json'): |
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.
Is anything else in this folder? It seems odd to me to check for the file extension when that isn't sufficient to guarantee that we have a well-formed bug report. I guess it's a good first cut though if someone mangles the BUG_REPORT_PATH
bin/post-issues
Outdated
formatter_class=fclass) | ||
parser.add_argument('-u', '--user', dest='username', | ||
default=None, | ||
help="Username for GitHub account") |
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.
It looks like this is an optional argument, which is odd because we don't prompt the user for it if omitted. I expect the script breaks if this isn't passed.
docs/source/user_utils.rst
Outdated
without valid GitHub accounts. Once issues are received on GitHub the | ||
appropriate action will be made by the PCDS staff. This may mean a deeper look | ||
at the linked log files and/or creating a distilled issue or action item in a | ||
different repository. |
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.
None of the bug reporting stuff belongs on this page, it's not a user utility that you'd include in a beamline file. It deserves it's own page before this one on the index.
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.
Didn't realize this was only for beamline.py utilities. Might suggest a rename if this was your intention.
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.
I agree, the page should be renamed. Even if it would fit on the page, bug reporting still deserves it's own page because of how important it is.
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.
Makes sense. Bug Reporting
page has been created 👍
hutch_python/bug.py
Outdated
""" | ||
One advantage of standardizing the logging system and startup scripts with | ||
hutch-python is programatically being able to gather information about the | ||
currrent Python environment. One common use case for much of this information |
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.
currrent
typo
hutch_python/bug.py
Outdated
# If the user somehow has no IPython history this is raised. A very rare | ||
# occurence except for in Continuous Integration tests | ||
except OSError: | ||
logger.exception |
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.
What does this do without a call or inputs?
hutch_python/bug.py
Outdated
try: | ||
logfiles = get_session_logfiles() | ||
except RuntimeError: | ||
logger.warning("No debug RotatingFileHandler configured for session") |
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.
Surely not every RuntimeError
is because the rotating file handler is missing? I feel like get_session_logfiles()
should return an empty list instead of raising an error when there are no session logfiles.
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.
This was a hold over from the get_console_handler
functionality.
hutch_python/bug.py
Outdated
@line_magic | ||
def report_bug(self, line): | ||
"""Creates a bug_report while running the given line""" | ||
# Enter both the DEBUG context and store the output of our command |
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.
Is this comment true?
hutch_python/tests/test_bug.py
Outdated
return r | ||
|
||
|
||
def test_bug_report(monkeypatch): |
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.
didn't realize this monkeypatch
fixture existed, neat trick
bin/post-issues
Outdated
logger.exception("Error posting %s", issue) | ||
|
||
|
||
if __name__ == '__main__': |
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.
Can you add a test that runs this script? Look for my tests that shell out and call hutch-python
. Alternatively, move main
into bug.py
and test it there.
This is ready for review again. |
path = os.path.join(BUG_REPORT_PATH, '{}.json'.format(str(uuid.uuid4()))) | ||
logger.info("Saving JSON representation of bug report %s", path) | ||
simplejson.dump(report, open(path, 'w+')) | ||
return path |
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.
Did you change your mind about making this skip the save file step?
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.
Never mind, you wanted to do this in a separate PR.
I'm still uneasy about the low coverage. If you'd like, I'll suggest lines that should be covered. |
Fair retort. The major hole is the |
Added two additional tests for the specifics of I left the |
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.
👍
Description
The idea of this PR is that user should be able to report a bug from the IPython session that will eventually make its way to Bug-Reports. Because we have standardized the way that we organize logs, CONDA environments e.t.c we can tell a lot about the current Python environment by just poking the right environment variables e.t.c.. To see the polished example of what the bug report will look like checkout https://github.com/teddyrendahl/Bug-Reports/issues/1. The process is kind of multi-step so I figure I'd give a quick walkthrough here. Everything is kicked off when the user call the
report_bug
function. This can either be used as an IPython magic to report a bug in a single line of code, or run as a regular function where you can grab the lastn
commands from the shell.We then ask the operator some brief questions about the issue we will not be able to determine programatically.
A
vim
window with a little prompt is also opened up so that the operator can type a more detailed explanation of the issue that looks likeNow the major issue is that we can't expect every operator to have a GitHub account so instead when the issue is created it gets stored in a JSON file in a known location in NFS. Once this is ready for ops we can setup a CRON job with a reasonable frequency to scan the directory, find new issues and post them to GitHub. This will probably be done with a
pcdsbot
account I'll setup. Thepost_to_github
function is there when this is ready to go, and I also created the full script ashutch-python/bin/post-issues
Motivation and Context
Closes #64
How Has This Been Tested?
Tests added for relevant top-level functions.
Where Has This Been Documented?
Added this to the
Useful Utilities
page of the User Documentation. Also updated theInternal API Reference