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

add junit output and summaries for benchmarks #16660

Merged
merged 41 commits into from
Oct 25, 2023
Merged

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Oct 19, 2023

@altendky altendky added Added Required label for PR that categorizes merge commit message as "Added" for changelog CI CI changes labels Oct 19, 2023
@coveralls-official
Copy link

coveralls-official bot commented Oct 23, 2023

Pull Request Test Coverage Report for Build 6633372508

  • 24 of 149 (16.11%) changed or added relevant lines in 6 files are covered.
  • 14 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.1%) to 90.006%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/core/test_cost_calculation.py 0 1 0.0%
tests/wallet/test_offer_parsing_performance.py 0 2 0.0%
tests/core/full_node/test_performance.py 0 3 0.0%
tests/conftest.py 12 17 70.59%
tests/util/misc.py 12 23 52.17%
tests/process_benchmarks.py 0 103 0.0%
Files with Coverage Reduction New Missed Lines %
chia/server/node_discovery.py 1 78.01%
tests/util/misc.py 1 61.75%
chia/farmer/farmer_api.py 2 87.14%
chia/full_node/full_node_api.py 2 77.24%
chia/timelord/timelord.py 2 73.64%
chia/wallet/wallet_node.py 6 87.51%
Totals Coverage Status
Change from base Build 6632571877: -0.1%
Covered Lines: 93152
Relevant Lines: 103435

💛 - Coveralls

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 24, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@aqk
Copy link
Contributor

aqk commented Oct 24, 2023

--benchmark-repeats is a tiny bit confusing (eg. does it mean num_benchmark_repeats, or do_benchmark_repeats)
https://github.com/Chia-Network/chia-blockchain/pull/16660/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R378

@aqk
Copy link
Contributor

aqk commented Oct 24, 2023

You could be a little less fancy here - keep the regex match, then just str.replace() [ and ]
https://github.com/Chia-Network/chia-blockchain/pull/16660/files#diff-4d8b94eb59960fd8e48cf3187b1c63427acef34c56b07ee835ec665288a66096R122

@altendky
Copy link
Contributor Author

--benchmark-repeats is a tiny bit confusing (eg. does it mean num_benchmark_repeats, or do_benchmark_repeats) #16660 (files)

Oy, I did not add help text for the --benchmark-repeats option. Would that be good? Or do you want a different name.

@altendky
Copy link
Contributor Author

You could be a little less fancy here - keep the regex match, then just str.replace() [ and ] #16660 (files)

I don't quite follow, but also this code is... not exactly clear about the purpose. It is meant to take the parametrization of the form [a-b-c] for the three parameter ids a, b, and c and remove a parametrization id of the form benchmark_repeat000 regardless of it being the only item, first item, middle item, or last item. Basically to get back to the parametrization as it would be presented if there were no benchmark repeat fixture parametrization in use. All without ending up with -- or [- or -]. But maybe that's what you're saying. Let those happen and then just do three replaces to catch those cases?

I probably ought to just do this on the test run side of things and then record that as another property in the results. I expect I would be able to do it there without any string manipulation.

@aqk
Copy link
Contributor

aqk commented Oct 24, 2023

Ya, I was just saying you didn't have to do the match and sub at the same time, and that might help sidestep the issue you were commenting on.

@aqk
Copy link
Contributor

aqk commented Oct 24, 2023

Adding a docstring --benchmark-repeats should be plenty!

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 24, 2023
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions
Copy link
Contributor

@cmmarslender cmmarslender merged commit 311d014 into main Oct 25, 2023
252 of 253 checks passed
@cmmarslender cmmarslender deleted the benchmarks_junit branch October 25, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog CI CI changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants