Refactor tests to separate test plan definitions and backend tests

[gaurav-arya]

The main purpose of this PR is to separate the definition of test plans from the FFT tests themselves, so that the latter can be run without the former. Some points:

• I wrapped the FFT backend tests into a function so that it can later take backend-specific arguments. For example, type of array (edit: done), or a backend tag once https://github.com/JuliaMath/AbstractFFTs.jl/issues/32 is addressed (and also a Union type for the InnerPlan's will very soon need to be passed in for the tests in https://github.com/JuliaMath/AbstractFFTs.jl/pull/67 to work for different backends.).
• I chose to wrap both the plan definitions and backend tests into modules as this makes the dependency of each one more clear. ~~But per discussion in https://github.com/JuliaMath/AbstractFFTs.jl/pull/76, the tests module will not be its own package yet, but can be used in FFTW.jl via the following hack: https://github.com/JuliaMath/FFTW.jl/pull/249/commits/d57515f51a6bd30e392f3f739417b6d481133226. But I've structured this so that it'll be very easy to make it its own package and remove the hack later~~ The backend tests are a submodule at AbstractFFTs.TestUtils

[codecov[bot]]

Codecov Report

Patch coverage: 94.57% and project coverage change: -4.84% :warning:

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%)	:arrow_down:
ext/AbstractFFTsTestExt.jl	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[devmotion]

I think a test of the interface does not require AD tests (https://github.com/JuliaMath/AbstractFFTs.jl/pull/76#issuecomment-1229551356)? 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

[gaurav-arya]

@devmotion are you ok with merging this? It's an imperfect solution (e.g. we won't get TagBot for AbstractFFTsTestUtils), but given our use case (a test utilities package that is highly useful both for testing of the interface package and downstream packages) I think it's the most appropriate solution that avoids a) forcing AbstractFFTsTestUtils to live in a separate repo and development of interface+tests to often require separate PRs b) having two separate test actions where AbstractFFTs' ordinary package tests omit tests of key functionality.

[devmotion]

Actually, I started to dislike this subpackage approach more and more. Therefore I commented on https://github.com/JuliaIO/TranscodingStreams.jl/pull/133 to get some additional input.

[gaurav-arya]

A weak dependency is for a very particular situation that I don't think is satisfied here. Indeed, one could in theory want to call test_fft_backend without loading Test explicitly.

And I really don't think the include mechanism should be used for code loading between different repos (even if it's just for tests)

So an AbstractFFTsTestUtils had to exist in someway. One could put it in its own repo, and make a separate test action for testing TestPlans + AbstractFFTsTestUtils independent of AbstractFFTs own tests. I shied away from this because AbstractFFTs and AbstractFFTsTestUtils are fairly coupled (one will very often want to modify or at least run the latter when modifying the former), hence this PR, which does morally something similar but is a bit lazier about separating it all out.

But if you dislike this solution, we can:

• make an AbstractFFTsTestUtils package, that can even live in a separate repo since lib packages are also controversial.
• Add the test with TestPlans to its test suite.
• Add an integration test with AbstractFFTsTestUtils here. (Which would be a very important integration test!)

The above steps seem pretty reasonable to me too. What do you think?

[gaurav-arya]

@devmotion I've updated the PR to avoid any special tricks when it comes to Pkg.develop / inconsistent state. Instead, AbstractFFTsTestUtils is a completely separate package with its own tests, which we integration test here.

The design seems like the only possible one given that manually including a file is a non-standard way of importing external code, so I think it would very hard for me to walk up to CUDA.jl, AMD, etc. and convince them to do this in their tests, which leaves a separate package AbstractFFTsTestUtils as the only solution.

I think the ecosystem is moving away from the lib directory with subpackages, so for that reason I've made AbstractFFTsTestUtils a separate repo. For the purpose of this PR, I've hosted it at https://github.com/gaurav-arya/AbstractFFTsTestUtils.jl. However, it should really be registered under JuliaMath: if you agree with my reasoning on the path forward, I'd be grateful if you could assist in transfering the repo to the org, or alternatively create a blank repo at JuliaMath/AbstractFFTsTestUtils.jl and I could then PR my code there so it can be reviewed before merge. If you strongly feel it should be a subpackage in the same repo, I'd be happy to stick with that too.

Ultimately, all of this is just trying to get towards finishing up chain rules for plans: see https://github.com/JuliaMath/AbstractFFTs.jl/issues/94 for the overall plan.

[devmotion]

I think a completely separate package is a suboptimal approach since it will be problematic to keep them in sync - changes to the API here mean tests could start to fail and a new release of the test package would be needed which however can only be done once a new release of the main package is available... So, no, I don't think separating them is a good idea (and I've encountered such issues in other packages, so I don't think this is a hypothetical scenario).

Maybe we should just add it as a submodule. I don't think the annoyances and problems with the alternatives are worth it, only to avoid a dependency on the Test stdlib.

[gaurav-arya]

I could revert to the submodule -- my worry there is that if we come up with a better solution in the future, we couldn't remove the submodule from AbstractFFTs without a breaking change. And I don't think a breaking change would be nice because the package has 48 dependents the majority of which wouldn't be using the test utilities.

Do you think marking it as experimental (and having FFTW and other downstream packages call this experimental API in their tests) would be a satisfactory approach, to avoid having to make a breaking change in the future? If so, then I'm happy to go for the submodule solution for now :)

[gaurav-arya]

Also, I just want to make sure I understand the issue you pointed out with a separate package. If I'm understanding right, it would occur if a breaking change was made to AbstractFFTs in the future. In that case:

1. the downstream testing of AbstractFFTsTestUtils that is run here would fail (note that in the current solution, AbstractFFTsTestUtils is not used in the main tests of this package, and is not a dependency of any form in the Project.toml)
2. we would have to merge the breaking change here with downstream tests failing (as usual for a breaking change if I understand correctly?)
3. we would then update AbstractFFTsTestUtils to bump to the new breaking version of AbstractFFTs and update the tests accordingly, and all the tests would then pass. (And in practice we would likely make the two PRs concurrently and at least via local testing we would know that the tests are all going to pass after both PRs merge, before merging anything.)

To me, this workflow doesn't sound too bad. (And a breaking change to AbstractFFTs is of course unlikely.) But I'll defer to whatever you think the best approach to follow is, and whichever solution you think is most acceptable for now:) My priority for now is just to get this merged with some acceptable solution, so I can help support differentiation of plans. Thank you for taking the time to review!

[devmotion]

And in practice we would likely make the two PRs concurrently and at least via local testing we would know that the tests are all going to pass after both PRs merge, before merging anything.

The main issue is that it relies on local testing and there's no way to check it via CI.

I think a submodule (no separate package), possible with an extension on >= 1.9 (if it is a reasonable choice, see e.g. https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/pull/521), is the simplest solution - and used by other packages as well.

[gaurav-arya]

@sethaxen I saw that you've been working on test utilities for Enzyme (https://github.com/EnzymeAD/Enzyme.jl/pull/782), so I was wondering if you had input on the best design here for AbstractFFTsTestUtils, between a submodule, weak dep, and separate package? I'm happy to push this through once there's consensus.

[gaurav-arya]

@devmotion @sethaxen This is now ready! The test suite includes tests of adjoint functionality which would hopefully be helpful downstream (see https://github.com/JuliaMath/AbstractFFTs.jl/issues/94, I've tested that the test suite also works fine with FFTW.)

[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.)

[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 https://github.com/JuliaMath/FFTW.jl/pull/249 with https://github.com/JuliaMath/AbstractFFTs.jl/pull/78 and https://github.com/JuliaMath/AbstractFFTs.jl/pull/109, so FFTW rules are ready to go. See also the tracking issue: https://github.com/JuliaMath/AbstractFFTs.jl/issues/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.