Diffractor.jl icon indicating copy to clipboard operation
Diffractor.jl copied to clipboard

Take testing seriously

Open mcabbott opened this issue 3 years ago • 5 comments
trafficstars

This adds many tests from Zygote. I've done some filtering to leave out the most repetitive ones, things which clearly don't apply (like Params), and things testing rules for external packages.

All which didn't pass are marked broken... ~~with (the local version of) #68. Thus many will fail.~~ Some have the error copied next to them. Exactly two have SILENTLY WRONG ANSWER in the comment (and should probably become issues) but this is not automatically distinguished from just an error.

~~Probably we should move the existing tests here into another file or two, to be a bit more organised.~~ (done) ~~I also had somewhere a few test cases of rules in ChainRules which are meant to support higher derivatives, but not very well tested there to do so.~~ (some added)

Closes #67, by adding the minimal piracy needed to allow Pairs to have a NamedTuple gradient (since many others tests failed without this).

mcabbott avatar Jan 12 '22 22:01 mcabbott

ForwardDiff and TaylorSeries are also sources of tests since we have and want to explicitly support forward mode.

Keno avatar Jan 12 '22 22:01 Keno

Good point. Ideally as many as possible of these would run all ways. The extent of my attempt at this so far is these lines , which ran into https://github.com/JuliaDiff/Diffractor.jl/issues/70 .

Edit: All of ForwardDiff's tests are copied here, and have loops set up to run them with all combinations of reverse jacobain over forward Hessian, etc. But the exotic combinations mostly don't work at the moment.

mcabbott avatar Jan 12 '22 23:01 mcabbott

These tests really want a different @test_broken macro, which accepts an error as a broken test, but fails on false -- a silently wrong answer is much worse than an error. Has anyone got one in their desk drawer?

mcabbott avatar Jan 15 '22 19:01 mcabbott

I looked into this a while back for an unrelated issue, but it seems the functionality behind those macros is not terribly extensible. Perhaps worthy of creating a TestMacros or TestExtras package if one doesn't already exist?

ToucheSir avatar Jun 29 '22 17:06 ToucheSir

Codecov Report

Merging #73 (1250a47) into main (55d2871) will increase coverage by 2.42%. The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   55.54%   57.96%   +2.42%     
==========================================
  Files          21       21              
  Lines        2157     2172      +15     
==========================================
+ Hits         1198     1259      +61     
+ Misses        959      913      -46     
Impacted Files Coverage Δ
src/runtime.jl 78.26% <66.66%> (-1.74%) :arrow_down:
src/extra_rules.jl 57.98% <100.00%> (+10.27%) :arrow_up:
src/stage1/recurse.jl 96.25% <0.00%> (-0.86%) :arrow_down:
src/interface.jl 70.90% <0.00%> (ø)
src/stage1/recurse_fwd.jl 94.28% <0.00%> (ø)
src/stage1/generated.jl 78.47% <0.00%> (+0.19%) :arrow_up:
src/stage1/forward.jl 80.13% <0.00%> (+5.98%) :arrow_up:
src/tangent.jl 60.00% <0.00%> (+25.95%) :arrow_up:
src/stage1/broadcast.jl 92.30% <0.00%> (+92.30%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Sep 18 '22 18:09 codecov-commenter