-
Notifications
You must be signed in to change notification settings - Fork 383
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
[analyzer] Generate reproducer #3324
Conversation
self.assertEqual(archived_code.read().decode("utf-8"), | ||
source_code.read()) | ||
|
||
os.remove(os.path.join(reproducer_dir, reproducer_files[0])) |
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.
Couldn't we remove the whole reproducer directory here?
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 copy-pasted it from test_failure()
. But I removed the whole directory at both places now.
@@ -99,7 +99,8 @@ usage: CodeChecker check [-h] [-o OUTPUT_DIR] [-t {plist}] [-q] | |||
[--report-hash {context-free,context-free-v2}] | |||
[-i SKIPFILE | --file FILE [FILE ...]] | |||
[--analyzers ANALYZER [ANALYZER ...]] | |||
[--capture-analysis-output] [--config CONFIG_FILE] | |||
[--capture-analysis-output] [--generate-reproducer] |
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.
Please update the help output of the CodeChecker analyze command too.
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.
Done
if generate_reproducer: | ||
handle_reproducer( | ||
source_analyzer, rh, | ||
os.path.join(reproducer_dir, result_base + '.zip'), | ||
actions_map) | ||
else: | ||
handle_failure( | ||
source_analyzer, rh, | ||
os.path.join(failed_dir, result_base + '.zip'), | ||
result_base, actions_map) |
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 pattern is repeated a couple of times. Maybe we can create a separate function or something similar for it:
def handle_analysis_(success, generate_reproducer, ...):
""" TODO """
if success:
handle_success()
if generate_reproducer:
handle_reproducer()
elsif not success:
handle_failure()
And we can call this function at least 4 places.
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 can't see the advantage of this extra function. This would require a function with as many parameters as handle_success()
, handle_failure()
and handle_reproducer()
have altogether, plus another which indicates whether it was a success or failure. Personally I think it doesn't help readability to introduce one more handler function where we have to follow through how to handle the result of an analysis. What do you think?
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.
Yes, you are right, this function would contain a lot of parameters:
def handle_analysis_return(success, generate_reproducer, source_analyzer,
rh, zip_file, actions_map, result_base, skip_handler,
capture_analysis_output, success_dir):
...
But what if we create an inner function inside this check function:
def check():
source_analyzer = ...
rh = ...
def handle_analysis_return(success, zip_file):
""" MY HELPER FUNCTION """
pass
if rh.analyzer_returncode == 0:
handle_analysis_return(True, reproducer_zip_file)
What do you think about this solution? I just want to avoid so much code duplications.
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.
Done
@@ -740,13 +764,19 @@ def signal_handler(signum, frame): | |||
if not os.path.exists(success_dir): | |||
os.makedirs(success_dir) | |||
|
|||
# Similar to failed dir, but generated both in case of success and failure. | |||
reproducer_dir = os.path.join(output_path, "reproducer") |
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.
If I run the analysis without the --generate-reproducer
option it will create an empty reproducer
directory in my report directy. I think if this option is not specified we can skip creating this directory.
|
||
ctu_zip_file = result_base + ctu_suffix + failure_type + '.zip' | ||
ctu_zip_file = os.path.join(failed_dir, ctu_zip_file) | ||
failed_zip_file = os.path.join(failed_dir, zip_file) |
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.
When there is no failure when analysing my project and I use the --generate-reproducer
option it will create zip files inside my report directory, but it will contain the word unknown
: main.cpp_clangsa_cf726ed90650ca7a768d4364b14ccd3c.plist_unknown.zip
. The reason is that the failure_type
default value is _unknown
even if the analysis was successfull. In case of failue maybe we can set it to an empty string.
CodeChecker analyze has been added a new flag: --generate-reproducer. This creates .zip files under report_dir/reproducers which contains all information for reproducing an analysis. The content of these .zip files is the same as failed zips, but reproducers are generated even in case of success.
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!
CodeChecker analyze has been added a new flag: --generate-reproducer.
This creates .zip files under report_dir/reproducers which contains all
information for reproducing an analysis. The content of these .zip
files is the same as failed zips, but reproducers are generated even in
case of success.