Refactor tests: R2N

[MaxenceGollier]

@dpo @MohamedLaghdafHABIBOULLAH, related to #130.

This is very much WIP, but you can already take a look.
I did a proof of concept with R2N, all solvers will eventually have similar tests.

• There should be one function that does most of the test, this function is test-solver.jl
• each solver should have its own test file
• allocations test don't need to be separated from the other tests
• Just testing if the algorithm completes on the bpdn problem is not sufficient
• having verbose > 0 adds too much information/noise into the test results

[MaxenceGollier]

LSR1 is allocating, will try to fix in LinearOperators.jl

[MohamedLaghdafHABIBOULLAH]

Thank you @MaxenceGollier very thoughtful, having the algorithms tested on different problems (toys, bpdn and others) will be very useful!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.72%. Comparing base (e0f214d) to head (ead3fc5).
⚠️ Report is 268 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #271       +/-   ##
===========================================
+ Coverage   61.53%   84.72%   +23.19%     
===========================================
  Files          11       13        +2     
  Lines        1292     1624      +332     
===========================================
+ Hits          795     1376      +581     
+ Misses        497      248      -249     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MaxenceGollier]

@dpo, @MohamedLaghdafHABIBOULLAH,

maybe we merge this first and then do the other solvers ? Or do I just do everything at once here ?
Let me know.

[MohamedLaghdafHABIBOULLAH]

@MaxenceGollier I think you should keep this PR tested only with R2N, so that it is easy to review and we will add the other solvers in different PR!

[MohamedLaghdafHABIBOULLAH]

could you open an issue for this comment?

[MaxenceGollier]

I think it is just a bad idea to add the allocations tests inside this function because this means that you can not assign a variable to one of the solver_constructor_kwargs or solver_kwargs, or else they will get boxed and result in allocations.

To be clearer @wrappedallocs works in such a way that even if

@wrappedallocs solve!(solver, reg_nlp, stats; atol = 1e-3)

returns 0,

tol = 1e-3
@wrappedallocs solve!(solver, reg_nlp, stats; atol = tol)

doesn't.

I think adding this constraint is very confusing so we should just leave the allocation tests out of this function.
I want to keep the comment though so that users will understand why we did not add the allocation tests there, but it might be more precise.

[MohamedLaghdafHABIBOULLAH]

Can't we have a function wrappedallocsv2 to avoid this?

[MaxenceGollier]

No, there is nothing "easy" we can do, keyword arguments will get boxed, this is a Julia issue.

[MohamedLaghdafHABIBOULLAH]

@MaxenceGollier
I checked this in detail. The issue is actually related to R2DH, and we should probably open a dedicated issue. The problem occurs when $$s = 0$$. In this case, R2DH does not stop because $$\xi = h_k - m_k(s) + \max(1, |h_k|)\times 10 \times \varepsilon = \max(1, |h_k|)\times 10 \times \varepsilon.$$
Although this quantity may be small, it is not negligible. When we compute its root, the resulting value can be larger than expected, which may prevent the algorithm from terminating.

A simple and reasonable fix is to assume instead that $$\xi = h_k - m_k(s).$$

[MaxenceGollier]

I never understood why we added this term to $$\xi$$ @dpo ?

[dpo]

That term is there to compensate for catastrophic cancellation between $h_k$ and $m_k(s)$. It’s hard to believe that a solver would compute $s = 0$ exactly. That suggests that the solver should have stopped earlier.

@MohamedLaghdafHABIBOULLAH Could you please show the run of R2DH where you observed $s = 0$?

[MaxenceGollier]

@MohamedLaghdafHABIBOULLAH, I agree with Dominique, could you provide us a run where this occurs ?

[MohamedLaghdafHABIBOULLAH]

Sorry I just saw the message, yes of course.

[dpo]

This looks great, thank you! I agree that we can have this PR focus on R2N only, and open future PRs for other solvers.

[dpo]

Testing for allocations would be the main point I think, because the first set of test (where you call solver_fun) already call solver_contructor and solve!(). I wonder if something like @wrappedallocs @eval solve!(solver, reg_nlp, stats; atol = $atol) would work. I agree that allocation tests need not be in this function, but, in general, it should be possible to construct the expression of the solve call and measure allocations.

[Copilot]

Pull request overview

Refactors the test suite to centralize shared test helpers and introduce a proof-of-concept, solver-focused test file for R2N, aligned with the broader effort in #130 to reorganize unit tests.

Changes:

• Add test/utils.jl with shared test utilities (allocation macro + Rosenbrock problem builder).
• Add test/test-solver.jl helper to run a common set of assertions for both the “allocating” and “optimized” solver call paths.
• Add test/test-R2N.jl with R2N-specific tests (basic statuses + BPDN scenarios), and wire it into runtests.jl.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/utils.jl	Introduces shared allocation macro and Rosenbrock test problem construction.
test/test_allocs.jl	Removes duplicated @wrappedallocs macro now provided by test/utils.jl.
test/test-solver.jl	Adds a centralized helper function for solver behavior validation across call paths.
test/test-R2N.jl	Adds dedicated R2N test coverage for multiple termination statuses and BPDN setups.
test/runtests.jl	Imports ManualNLPModels and includes the new shared utilities and R2N test file.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.