Codecov Report

Merging #37 (2c2b5d0) into main (38b1a3d) will increase coverage by 4.98%. The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
+ Coverage   75.61%   80.59%   +4.98%     
==========================================
  Files           9        9              
  Lines         570      567       -3     
==========================================
+ Hits          431      457      +26     
+ Misses        119       89      -30     
- Partials       20       21       +1

Impacted Files	Coverage Δ
proc.go	`67.93% <50.00%> (+14.99%)`	: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 38b1a3d...6ea7f0e. Read the comment docs.

sbinet commented 3 years ago

PTAL @psampaz

psampaz commented 3 years ago

@sbinet it looks pretty fine. It is much simpler that my version so I will close https://github.com/go-p5/p5/pull/35 in favour of this one.

I have only one comment about a naming issue that caused me confusion when I first read the test code. testProc actually handles a specific use case. Draw once, grab a screen shot and compare it with the golden file. If you think it makes sense I would suggest to rename it to something more specific that indicates its purpose, in this or another PR.

sbinet commented 3 years ago

You're right. I think that could be addressed by modifying the 'Run' signature to take a possibly nil chan of event.Event that testProc.Run would consume.

This might allow to tackle the use case you exposed in the other pr.

go-p5 / p5

all: re-wire event channel for testability #37

Codecov Report