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

feat: Errors thrown no longer fail entire benchmark process #166

Merged
merged 11 commits into from
Jul 11, 2023

Conversation

ORyanHampton
Copy link
Contributor

@ORyanHampton ORyanHampton commented Jun 30, 2023

Description

When running benchmarks, if even one failed, we would lose all collected data.
These changes output collected benchmarks and drop the data from failed benchmarks. This helps prevent data loss on benchmarks that aren't failing.

How Has This Been Tested?

Ran locally and checked output

Minimal checklist:

  • I have performed a self-review of my own code
  • I have added DocC code-level documentation for any public interfaces exported by the package
  • I have added unit and/or integration tests that prove my fix is effective or that my feature works

@ORyanHampton ORyanHampton changed the title Errors thrown no longer fail entire benchmark process feat: Errors thrown no longer fail entire benchmark process Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #166 (ea88579) into main (9cc05dc) will not change coverage.
The diff coverage is n/a.

❗ Current head ea88579 differs from pull request most recent head f71da60. Consider uploading reports for the commit f71da60 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #166   +/-   ##
=======================================
  Coverage   71.42%   71.42%           
=======================================
  Files          26       26           
  Lines        3446     3446           
=======================================
  Hits         2461     2461           
  Misses        985      985           

see 1 file with indirect coverage changes

see 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3b66be...f71da60. Read the comment docs.

Copy link
Contributor

@hassila hassila left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Agree it would be nicer to not drop the rest of the results, but the suggested solution would break CI workflows, so needs to be reworked a bit, see comments.

Plugins/BenchmarkTool/BenchmarkTool.swift Show resolved Hide resolved
Plugins/BenchmarkTool/BenchmarkTool.swift Show resolved Hide resolved
Plugins/BenchmarkTool/BenchmarkTool.swift Show resolved Hide resolved
Plugins/BenchmarkTool/BenchmarkTool+Export.swift Outdated Show resolved Hide resolved
Plugins/BenchmarkTool/BenchmarkTool+Operations.swift Outdated Show resolved Hide resolved
Plugins/BenchmarkTool/BenchmarkTool.swift Show resolved Hide resolved
Plugins/BenchmarkTool/BenchmarkTool.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@hassila hassila left a comment

Choose a reason for hiding this comment

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

Super, LGTM.

Thanks for the contribution!

@hassila hassila merged commit 9e010e2 into ordo-one:main Jul 11, 2023
hassila added a commit that referenced this pull request Dec 20, 2023
…ilures (#211)

After the changes in #166 we no longer fail the run of the rest of the benchmarks if a single one fails, which is reasonable during normal iterative work. This is not ok for CI baseline checks though, so this PR restores failures when `baseline check` is performed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants