Check for log regression #406
Conversation
mccheah
left a comment
mccheah
left a comment
There was a problem hiding this comment.
Might be able to filter on lines containing logDebug, logWarning, logInfo, logError in the diff?
.circleci/config.yml
Outdated
| path: /tmp/publish_artifacts.log | ||
| destination: publish_artifacts.log | ||
|
|
||
|
|
|
|
||
| success = True | ||
| for log_file in files_to_check: | ||
| output = subprocess.check_output(['git', 'diff', 'master', log_file]) |
There was a problem hiding this comment.
Is part of the workflow to run this locally? If so, for those of us (myself included) who track multiple remotes, master is ambiguous here. Might want to pass the master branch to diff against as a command line argument and then default to master, then in CI we just put master or origin/master.
|
Yea I was thinking about that, but there could be multiline logs, so it's not a simple grep... let me see if I can write up some simple parser |
|
@mccheah ok i attempted to parse out only the log lines. i also made it so it only checks for modified or added log lines and not deleted ones |
|
We should write a test around this with some sample diffs, see if the changes in logging are caught. |
|
|
||
| def _extract_log_line(file_lines, log_type, start_index): | ||
| log_line_string = " " + file_lines[start_index].strip() | ||
| if not log_line_string.endswith("+"): |
There was a problem hiding this comment.
Tricky - this won't catch triple-quoted multi line logs.
|
@vandervecken please don't post internal links on the public internet; I deleted your comment |
| def _extract_variables(file_parser, log_type): | ||
| # Find the start of the log line | ||
| active_stack = [] | ||
| while True: |
There was a problem hiding this comment.
Instead write as while (char != "{") - avoid while true unless intending to make an infinite loop.
| elif char == "\"": | ||
| return variables | ||
|
|
||
| def _parse_variable_outside_quotes(file_parser, close_chars): |
There was a problem hiding this comment.
We can present the whole log line that changed instead of just the variable. Would remove the need for these functions.
Basically just diff the argument of logging calls - but diff it from the in-code representation.
There was a problem hiding this comment.
Talked offline, pairwise comparison line by line results in confusing output when new logging statements are added. We'll instead diff on the set of arguments of all log statements between the old version of the file and the new version of the file, irrespective of the placement of the logging statements themselves.
|
ping @mccheah |
|
Wanna close this in favour of #425 ? |
This is how it works:
files-to-inspect.jsoncontains the list of current files to check inmaster. If you make a PR that changes one of those files, you can just add it tounmergedin that file. A PR merge will trigger the script to squash theunmergedandmasterlists into a singlemasterlist and empty out theunmergedlist. A release will trigger adding that release's listed files tofiles-to-inspect-archive.jsonso we can have an archive of all the files for every release.Happy to iterate on structure and functionality