lambdaisland / kaocha

Full featured next gen Clojure test runner
https://cljdoc.org/d/lambdaisland/kaocha/
Eclipse Public License 1.0
799 stars 84 forks source link

Add specs for profiling plugin data #302

Closed robhanlon22 closed 2 years ago

robhanlon22 commented 2 years ago

Plugin authors (like myself) might want to consume profiling data as part of their plugin. Having these specs available is useful for those authors so they can write their own specs against this data without having to violate boundaries and define the specs themselves.

codecov[bot] commented 2 years ago

Codecov Report

Merging #302 (e206727) into main (f97d9fd) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #302      +/-   ##
==========================================
+ Coverage   75.53%   75.57%   +0.03%     
==========================================
  Files          51       51              
  Lines        2714     2718       +4     
  Branches      252      253       +1     
==========================================
+ Hits         2050     2054       +4     
  Misses        506      506              
  Partials      158      158              
Flag Coverage Δ
integration 57.06% <75.00%> (+0.02%) :arrow_up:
unit 69.57% <100.00%> (+0.04%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/kaocha/plugin/profiling.clj 35.93% <100.00%> (+4.27%) :arrow_up:

Continue to review full report at Codecov.

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

alysbrooks commented 2 years ago

This is a good idea. I've been thinking about the possibility of cross-plugin dependencies and how we might want to handle them if they come up. The implementation also looks solid, although I'd want to actually compare it to the plugin code and kick the tires myself before merging.

Specs add a (small) cost to future changes and they also communicate that consumers are free to rely on the current shape of the data, so I'm not sure we want to set that expectation. @plexus, what do you think?

alysbrooks commented 2 years ago

To expand a little bit, I think my concerns could just as easily result in loosening up the specs a bit (like dropping ::profiling?) as rejecting this PR.

robhanlon22 commented 2 years ago

Or making ::profiling into an any? as a "declaration" that the configuration value is at least present?

plexus commented 2 years ago

No strong opinion for or against this, as long as we don't actually instrument any functions the overhead and risk should be small.

alysbrooks commented 2 years ago

Looking a bit into commit history, it looks like the keys were all present in July 2018, soon after the plugin was initially developed. I think we're pretty unlikely to change these at this point.

I'll kick the tires a bit, but then I think we can merge it!

alysbrooks commented 2 years ago

Thanks, @robhanlon22!

Animation of a green cartoon bean doffing its orange hat with "thanks!" written underneath