Put tests of FFT backends into TestUtils submodule

[gaurav-arya]

Puts tests that also stress test a particular FFT backend into a submodule TestUtils whose functionality can be used downstream.

[codecov]

Codecov Report

Patch coverage: 94.57% and project coverage change: -4.84% ⚠️

Comparison is base (1cc9ca0) 92.04% compared to head (6fec1b7) 87.20%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   92.04%   87.20%   -4.84%     
==========================================
  Files           3        4       +1     
  Lines         289      336      +47     
==========================================
+ Hits          266      293      +27     
- Misses         23       43      +20     

Files Changed	Coverage Δ
src/TestUtils.jl	0.00% <0.00%> (ø)
src/definitions.jl	69.04% <78.94%> (-14.99%)	⬇️
ext/AbstractFFTsTestExt.jl	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[devmotion]

I think a test of the interface does not require AD tests (#76 (comment))? I assumed one would only want to check whatever is required by the interface, i.e., basically what's written and required in the documentation of the interface.

[gaurav-arya]

That's a fair point. I had no special reason to depend on ChainRulesTestUtils other than specially checking AD for each backend, so I've tried to get rid of it and made a submodule with the relevant tests from the old test suite for checking FFT backends.

Edit: sorry this is still wip as I need to address the test failure when I get time, but the structure is otherwise there (ready for review now)

[gaurav-arya]

This is ready for a review!

[gaurav-arya]

Bump (looking for a review on this before the chain rules PRs; I'll adapt those after this has been merged)

[gaurav-arya]

@devmotion this is ready for another review.

One important point: it seems to be controversial to introduce a Test dependency into the main package. An alternative is to register a separate package AbstractFFTsTestUtils that would live in a subfolder in this repo. I can make the change if you think it's a good idea.

[gaurav-arya]

@devmotion bump since this is blocking the plan chainrules work (please also see my comment above about a potential AbstractFFTsTestUtils)

[devmotion]

Yeah, I was thinking about that. Given that generally the ecosystem tries to keep dependencies minimal, I agree that it's maybe not completely optimal to include and precompile them even if users don't need them. On the other hand, I don't think the PR in TranscodingStreams is the best approach either: If the test interface is available only in a test/testutils.jl file, this means that downstream packages have to know the filename of this file and also retrieve its path to load it.

I think there are a few alternatives:

• Putting it in a subpackage (something like lib/AbstractFFTsTestUtils) and registering this package
• Putting it in a file in tests/ but also define a global variable in the package itself that returns the path to the filename

Maybe there are more options? Maybe even an extension on Julia >= 1.9, but I'm not sure if/how that works with stdlibs?

I guess given the current situation, maybe a subpackage would be a good option? What do you think?

[gaurav-arya]

I think a subpackage is a better solution than including the files. I've made the move, but I'm currently working on updating the CI so that the AbstractFFTsTestUtils package is dev'd prior to the tests running. My current approach is to remove the julia runtest action and do it manually like the downstream tests, if you have any thoughts @devmotion I'd appreciate them:)

[gaurav-arya]

@devmotion I've made the subpackage, but I'm just not able to understand the error on Julia nightly. It's easily reproducible on 1.9 by trying to develop the AbstractFFTsTestUtils package from within the AbstractFFTs environment.

[gaurav-arya]

There's also a "Circular dependency detected" warning for seemingly no reason on Julia 1.8: AbstractFFTsTestUtils is only a test dependency.

[devmotion]

Hmm, I came to realize that it won't work this way even if tests would pass on Github since it means you would have to add AbstractFFTsTestUtils manually every time you want to run the tests locally. I see at least two options: We could move the tests with the test utilities to the subpackage and test the subpackage additionally (or only, if no tests are left in AbstractFFTs - but I don't think all tests exercise the FFT interface?); Or alternatively we could add the local version of AbstractFFTsTestUtils in the subfolder in test/runtests.jl. Generally I'm not a fan of messing around with packages in Julia scripts since this can lead to inconsistent states: If you already have an older/newer version loaded of some package, you have to restart the Julia process since otherwise the desired version is not used. And I've experienced such problems in multiple other projects. I guess here it could work though since AbstractFFTsTestUtils only depends on AbstractFFTs and Test, so no other packages should be affected.

[gaurav-arya]

I've implemented the develop-from-test-script solution here

[gaurav-arya]

@devmotion tests should be sorted now

[devmotion]

You can wrap it inside a if isdefined(Base, :get_extension).

[devmotion]

Outside of the whole __init__ function I meant, not in its function body.

[gaurav-arya]

Comments addressed, and I also refactored some repeated logic in the test utilities 🙂

[gaurav-arya]

Bump on review

[stevengj]

Couldn't we do a multi-repo like Makie.jl, where this repo contains both the AbstractFFTs package and an AbstractFFTTests package?

(In the long run, a better way to do testing is described at https://dsp.stackexchange.com/questions/633/what-data-should-i-use-to-test-an-fft-implementation-and-what-accuracy-should-i and is used in FFTW: it is sufficient to test linearity for random inputs, the unit-impulse response, and the time-shift property for random inputs.)

[devmotion]

Outside of the whole __init__ function I meant, not in its function body.

[vpuri3]

@gaurav-arya Test shouldn't be in deps, and weakdeps?

[gaurav-arya]

@vpuri3 confused me at first too, but that is how to migrate from a hard dep to a weak extension on 1.9+: https://pkgdocs.julialang.org/v1.9/creating-packages/#Transition-from-normal-dependency-to-extension

[gaurav-arya]

does this PR need anything more to merge? Please note that I have locally tested JuliaMath/FFTW.jl#249 with #78 and #109, so FFTW rules are ready to go. See also the tracking issue: #94

Regarding uncertaintities about weak extensions v.s. separate packages, etc: I have described the TestUtils module as experimental in the docs, and we will only be introducing it as a test dependency downstream. So I would rather go ahead with unblocking this line of work, and we have the freedom in the future to revise the decisions made here if necessary.

[gaurav-arya]

Merge conflicts resolved :)

[stevengj]

LGTM.

[gaurav-arya]

Could this get a release, to help with CI on downstream PRs?

[stevengj]

JuliaRegistries/General#88645