Skip to content
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

WT-12969 Generate per test code coverage results #10638

Merged
merged 28 commits into from
Jun 14, 2024

Conversation

jeremythorp
Copy link
Contributor

@jeremythorp jeremythorp commented May 24, 2024

This ticket adds Python code and an Evergreen task to generate per-task code coverage results and, for patch builds, create a simple report on which tests reach each added/changed WT function.

Copy link
Contributor

@jiechenbo jiechenbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an initial review, nice stuff btw!

# Logging for debugging
ls -l coverage_report
cat coverage_report/diff.txt
${python_binary|python3} test/evergreen/code_change_report/per_test_code_coverage_report.py -v -c coverage_data -d coverage_report/diff.txt -m coverage_report/metrixpp.csv
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can safely use python3 here, unless we are running this on a mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

test/evergreen.yml Show resolved Hide resolved
if not Path(gcovr_dir).is_absolute():
sys.exit("gcovr_dir must be an absolute path")

build_dirs = check_build_dirs(build_dir_base=build_dir_base, parallel=parallel_tests, setup=setup, verbose=verbose)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need for the equal sign, as we can just pass in the argument itself.

Copy link
Contributor Author

@jeremythorp jeremythorp Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, you're correct so I've removed them. But very useful sometimes.

with open(config_path) as json_file:
config = json.load(json_file)

if verbose:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks as tho, we would benefit from a logger class. We pass in the verbose variable into the class which would check the if condition instead, such that whenever we do something like:
logger.message("hello world"), it would check the verbose variable if it should actually print to stdout or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - I've introduced a logger.

git_diff_file = args.git_diff_file
complexity_data_file = args.metrix_complexity_data

if verbose:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this would also benefit from a logger class. The idea is that we put in verbose variable, and the class would check if verbose is set, if so, we would print the message. E.g. (logger.message("hello world"), would not print if verbose is set to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - I've introduced a logger.

@jeremythorp
Copy link
Contributor Author

Hi @jiechenbo and @ajmorton , I've updated the branch/PR based on Jie's feedback. Please let me know what you think.

Copy link
Collaborator

@ajmorton ajmorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 A few small suggestions but nothing the needs doing

test/evergreen.yml Outdated Show resolved Hide resolved
Comment on lines 39 to 40
info = json.load(json_file)
return info
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
info = json.load(json_file)
return info
return json.load(json_file)

Comment on lines +50 to +52
hunks = patch.hunks
hunk_list = list()
for hunk in hunks:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hunks = patch.hunks
hunk_list = list()
for hunk in hunks:
hunk_list = list()
for hunk in patch.hunks:

Comment on lines 108 to 113
change_info = dict()
change_info['status'] = hunk.status
change_info['new_start'] = hunk.new_start
change_info['new_lines'] = hunk.new_lines
change_info['old_start'] = hunk.old_start
change_info['old_lines'] = hunk.old_lines
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
change_info = dict()
change_info['status'] = hunk.status
change_info['new_start'] = hunk.new_start
change_info['new_lines'] = hunk.new_lines
change_info['old_start'] = hunk.old_start
change_info['old_lines'] = hunk.old_lines
change_info = { 'status': hunk.status, 'new_start': hunk.new_start, 'new_lines': hunk.new_lines, 'old_start': hunk.old_start, 'old_lines': hunk.old_lines }

Comment on lines 117 to 120
line_info = dict()
line_info['content'] = line.content
line_info['new_lineno'] = line.new_lineno
line_info['old_lineno'] = line.old_lineno
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
line_info = dict()
line_info['content'] = line.content
line_info['new_lineno'] = line.new_lineno
line_info['old_lineno'] = line.old_lineno
line_info = { 'content': line.content, 'new_lineno': line.new_lineno, 'old_lineno': line.old_lineno }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Comment on lines 213 to 225
diff_file = open(git_diff_file, mode="r")
diff_data = diff_file.read()
diff = Diff.parse_diff(diff_data)
change_list = diff_to_change_list(diff=diff, verbose=verbose)

complexity_data = read_complexity_data(complexity_data_file)
preprocessed_complexity_data = preprocess_complexity_data(complexity_data=complexity_data)

coverage_data = collate_coverage_data(gcovr_dir=coverage_data_path, verbose=verbose)

generate_report(coverage_data=coverage_data, change_list=change_list,
preprocessed_complexity_data=preprocessed_complexity_data, verbose=verbose)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
diff_file = open(git_diff_file, mode="r")
diff_data = diff_file.read()
diff = Diff.parse_diff(diff_data)
change_list = diff_to_change_list(diff=diff, verbose=verbose)
complexity_data = read_complexity_data(complexity_data_file)
preprocessed_complexity_data = preprocess_complexity_data(complexity_data=complexity_data)
coverage_data = collate_coverage_data(gcovr_dir=coverage_data_path, verbose=verbose)
generate_report(coverage_data=coverage_data, change_list=change_list,
preprocessed_complexity_data=preprocessed_complexity_data, verbose=verbose)
with open(git_diff_file, mode="r") as diff_file:
diff_data = diff_file.read()
diff = Diff.parse_diff(diff_data)
change_list = diff_to_change_list(diff=diff, verbose=verbose)
complexity_data = read_complexity_data(complexity_data_file)
preprocessed_complexity_data = preprocess_complexity_data(complexity_data=complexity_data)
coverage_data = collate_coverage_data(gcovr_dir=coverage_data_path, verbose=verbose)
generate_report(coverage_data=coverage_data, change_list=change_list,
preprocessed_complexity_data=preprocessed_complexity_data, verbose=verbose)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor

@jiechenbo jiechenbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jeremythorp jeremythorp added this pull request to the merge queue Jun 14, 2024
Merged via the queue into develop with commit 8f5f185 Jun 14, 2024
7 checks passed
@jeremythorp jeremythorp deleted the wt-12969-per-test-code-coverage branch June 14, 2024 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants