Consolidation of Test Mock Types and Helpers - #184
Open
vp314 wants to merge 3 commits into
Open
Conversation
…stants and refactor field macros - Delete `test_helpers/approx_tol.jl`; add `const ATOL = 1e-10` locally in the two files that actually use it (randsvd.jl, rangefinder.jl) - Remove unused `using ..ApproxTol` from all ~23 other test files - Rename `@test_projection_solver` → `@test_solver` with corrected common SolverRecipe fields (compressor/log/error/compressed_mat/mat_view/solution_vec, fixing incorrect field names S and update_vec from the old version) - Add `@test_sub_solver` macro for validating SubSolverRecipe field conformance Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add test/test_helpers/mock_types.jl with canonical, reusable mock type definitions, eliminating ~50 redundant struct definitions across 16 test files: - TestCompressor, TestApproximator, TestSolver: minimal stubs for type hierarchy tests - TestCompressorRecipe (n_rows, n_cols, status superset), TestApproximatorRecipe (n_rows, n_cols, code superset), TestSolverRecipe (code): cover all abstract interface test scenarios via a single shared type with multi-arg constructors - TestFullCompressor / TestFullCompressorRecipe: unified compressor mock with both 2-arg (Right/approximator) and 3-arg (Left/solver) complete_compressor overloads plus Left and Right mul! implementations Note: mul!, rapproximate!, complete_approximator, complete_solver, and rsolve! are intentionally absent from MockTypes — abstract-type error-phase tests require those functions to throw before any implementation exists. Files that define these methods locally extend MockTypes types explicitly (import ..MockTypes: TypeName) to satisfy Julia 1.12 qualification rules. Known: complete_approximator(::MockTypes.TestApproximator, A) is defined in both approximators_abstract_types and rapproximate_abstract_interface; Julia emits a method-overwrite warning. Tests pass. To be resolved in a follow-up. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Route the remaining per-file "Group 1" test mocks through shared MockTypes definitions, removing duplicated struct/method definitions. - Compressor mocks in randsvd.jl, rangefinder.jl, and range_finder_helpers.jl now reuse the existing (previously unused) TestFullCompressor / TestFullCompressorRecipe. - The divergent local TestSolverRecipe definitions in the ErrorMethods tests are replaced by a new shared TestFullSolverRecipe (a superset of the fields the compute_error routines read), with an all-optional keyword constructor so each test populates only what it exercises. Group 2 mocks (e.g. TestRangeApproximatorRecipe, TestSolverError) are intentionally left local for a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nathanielpritchard
requested changes
Aug 11, 2026
nathanielpritchard
left a comment
nathanielpritchard
left a comment
Member
There was a problem hiding this comment.
Everything seems to look good although the '@solver' macro checks for structures that are not necessarily in every solver. I believe that this macro should continue to be only for projection_solvers
Comment on lines
+72
to
+94
| @test_solver(type) | ||
|
|
||
| Macro for testing solver recipe types, such as `KaczmarzRecipe` and `IHSRecipe`. It checks | ||
| that every `SolverRecipe` includes the common interface fields `compressor::CompressorRecipe`, | ||
| `log::LoggerRecipe`, `error::SolverErrorRecipe`, `compressed_mat::AbstractMatrix`, | ||
| `mat_view::SubArray`, and `solution_vec::AbstractVector`. | ||
| """ | ||
| macro test_solver(type) | ||
| expr = quote | ||
| @testset verbose = true "Solver: $(string($(esc(type))))" begin | ||
| # Test the super type | ||
| @test supertype($(esc(type))) <: SolverRecipe | ||
|
|
||
| # Test the field names and types | ||
| for (fname, ftype) in SolverFields | ||
| @test fname in fieldnames($(esc(type))) | ||
| @test fieldtype($(esc(type)), fname) <: ftype | ||
| end | ||
| end | ||
| end | ||
|
|
||
| return expr | ||
| end |
Member
There was a problem hiding this comment.
I think that different solvers will not necessarily always feature these fields so we need to be more specific as to what types of solvers this macro is applied.
Member
There was a problem hiding this comment.
I still prefer the projection_solver version.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates the test suite's shared infrastructure to reduce duplication in mock definitions and test helpers.
Description
Refactors the test suite's shared infrastructure to reduce duplication of structs and functionality defined specifically for testing purposes. Not all structs and functionality were consolidated as this would require rewriting tests, which can be
done in the future.
Related changes:
ATOL = 1e-10module was removed in favor of a localconst ATOL = 1e-10in the two files that actually used it (randsvd.jl,rangefinder.jl). Removedusing ..ApproxTolfrom every test file.@test_solver(checks the commonSolverRecipeinterface:compressor,log,error,compressed_mat,mat_view,solution_vec) and@test_sub_solver(checksSubSolverRecipe'sAfield). Supertype checks now use<:instead of==to allow parametric recipe types.Motivation and Context
Many tests defined their own near-identical mock structs creating unnecessary code duplication. Centralizing the mocks makes the common recipe interfaces explicit in one place and reduces boilerplate across ~30 test files.
How has this been tested
The entire test suite was run using Julia's testing interface. All tests passed.
Types of changes
Checklists:
Code and Comments
If this PR includes modification to the code base, please select all that apply.
API Documentation
Manual Documentation
Testing
@code_lowered and
@code_typed)