Add downstream integration tests

[devmotion]

In the same way as in ChainRules, SciML, Turing etc. In contrast to #75 this avoids a circular dependency and does not cause test errors for breaking releases.

[codecov]

Codecov Report

Merging #76 (a58c065) into master (4b57f54) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #76   +/-   ##
=======================================
  Coverage   84.13%   84.13%           
=======================================
  Files           2        2           
  Lines         208      208           
=======================================
  Hits          175      175           
  Misses         33       33           

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

[devmotion]

It seems that AbstractFFTs master breaks FFTW. Probably caused by #72?

[gaurav-arya]

Should be fixed by #77.

I agree that the circular dependency is a bad idea. I'll still need to think of some way of testing adjoints of FFTW etc. so that the lines added in JuliaMath/FFTW.jl#249 can be tested (perhaps I should just copy and paste a subset of the adjoint test here into FFTW...), but agree that downstream tests are the right first step

[devmotion]

If you want to run the same tests for adjoint plans in downstream packages it might be useful to define a TestUtils submodule in AbstractFFTs and define the test interface there. Then they can be reused by downstream packages and they could be certain to implement the API correctly. See e.g. https://www.oxinabox.net/2020/04/19/Julia-Antipatterns.html#aside-test-suites-for-interface-testing and the implementation in https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/blob/master/src/TestUtils.jl, https://github.com/JuliaMath/InverseFunctions.jl/blob/master/src/test.jl, ChangesOfVariables etc.

[gaurav-arya]

That sounds like a great solution, I'll work on that. The downstream tests added here still make sense to merge though, e.g. for the error they caught this time, and because they'll eventually call the adjoint tests of FFTW etc once I've set that up

[gaurav-arya]

I got stuck on this because I worried that ChainRulesTestUtils is too heavy of a dependency to add for the TestUtils submodule of AbstractFFTs. What do you think? I could potentially add a few copy-pasted tests to FFTW instead, and wait to generalize later?

[stevengj]

I'm not so concerned about test dependencies, which don't affect ordinary users.

[gaurav-arya]

The issue here is that we were discussing running the test suite of this package as part of FFTW's tests, e.g. so that the trait definitions in JuliaMath/FFTW.jl#249 can be tested. The suggested approach was to put the suite in a submodule of the AbstractFFTs package itself. (Another solution would be to register a separate AbstractFFTsTestUtils package, but that also feels like a something that should be done when the test suite is much more mature than it is now?)

I'm tempted to just copy and paste a few tests from here to the FFTW test set instead to go along with JuliaMath/FFTW.jl#249, and wait to generalize later -- does that make sense?

[stevengj]

Can't we just do:

using AbstractFFTs
include(joinpath(pathof(AbstractFFTs), "..", "test", "runtests.jl"))

or similar?

The suggested approach was to put the suite in a submodule of the AbstractFFTs package itself.

I don't like the idea of imposing a runtime/loadtime cost on end users for the test suite.

[gaurav-arya]

I didn't think of the include trick. Although it's certainly hacky (there's a dependency on the internal file structure of AbstractFFTs), it seems like a reasonable solution to avoid registering the tests as their own package for now (I'm very happy to change the approach or even register the package right away depending on your thoughts on what's best).

#78 and JuliaMath/FFTW.jl@d57515f implement the suggested approach.

[devmotion]

@gaurav-arya can you explain why you would want to use ChainRulesTestUtils?

Generally, the idea with having a test suite for an interface (here and in other packages) is that downstream packages have a standardized and officially sanctioned way of checking if they implement the interface correctly. It's not supposed to cover every possible use case but a minimal set of tests that ensure that all necessary methods are implemented and all desired properties are satisfied. That should be possible without any additional dependencies, I think? If a downstream implementation defines the necessary traits, then AD should work automatically, I guess?