Fix @test_trixi_include_base to support locally-defined kwarg values

[JoshuaLampert]

Passing a locally-defined variable as a keyword argument override to @test_trixi_include_base previously failed with UndefVarError. The macro was collecting kwarg values as raw AST symbols and passing them to trixi_include, which then tried to resolve them as variable names inside TrixiTestModule, where local testset bindings don't exist.

The fix builds proper escaped keyword argument expressions. For bare-symbol values, it uses @isdefined at runtime to distinguish the two intended use cases: if the symbol is visible in the caller's scope (same world age), the actual value is captured; if not — which is the case for elixir-internal variable references, since Base.include creates bindings at a newer world age — the symbol is passed as-is for resolution inside the elixir.

Fixes #31.

AI helped me to build this PR, but it looks reasonable to me. However, since this is macro-magic, I would appreciate a second pair of eyes looking at this.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.94%. Comparing base (78aa58e) to head (4d0c5c4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/macros.jl	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
+ Coverage   98.86%   98.94%   +0.08%     
==========================================
  Files           3        3              
  Lines          88       95       +7     
==========================================
+ Hits           87       94       +7     
  Misses          1        1              

☔ View full report in Codecov by Harness.
📢 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.

[MarcoArtiano]

Thanks Joshua!

I have a minor comment. Should we remove the reference to your MWE?

[ranocha]

Thanks for working on this. I can't say for sure whether this is completely fine. Can you please add more tests to make sure to cover all branches in the more complex code?

[JoshuaLampert]

I think the tests should already test every branch and codecov also says there is nothing uncovered. In total we have four branches and they are (at least) covered by the following tests (I verified that locally by adding debug statements in the corresponding branch and running the isolated test code):

• Case 1 (val isa Symbol && val in kwarg_keys) is covered by

  TrixiTest.jl/test/test_test_trixi_include.jl

  Line 119 in bb66a14

  @test_trixi_include_base(path, seed=6, x=seed)

• Case 2 (elseif val isa Symbol), where @isdefined $val is true is covered by the newly added test https://github.com/trixi-framework/TrixiTest.jl/pull/32/changes#diff-1d0cff7a2d2c9c45c167d5ad36d367aca34383778b14e05b85982b7b155ca20bR152
• Case 2 (elseif val isa Symbol), where @isdefined $val is false is covered by

  TrixiTest.jl/test/test_test_trixi_include.jl

  Line 98 in bb66a14

  @test_trixi_include_base(path, x=seed)

• Case 3 (else) is covered by

  TrixiTest.jl/test/test_test_trixi_include.jl

  Line 43 in bb66a14

  @test_trixi_include_base(path, x=9)

[benegee]

Thanks for daring, brave boys!

[ranocha]

Thanks a lot! This looks good to me.

@vchuravy It would be great if you could take a look at the macro code as well.

[MarcoArtiano]

Does that also allow to pass nothing as value of a kwarg?

We can assign the value nothing the variables, but not overwrite functions with nothing, which is totally fine and I don't think the second case would ever be an actual useful case. For example with trixi_include we can set sol = nothing, and I find useful also to set sometimes some callbacks to nothing during tests (mainly because at the moment the GPU does not support some of the callbacks).

Note that, without this PR with @trixi_testset we cannot override variables to nothing values, while we can with trixi_include.

julia> @trixi_testset "debug.jl" begin
                         baz_override = nothing
                         @test_trixi_include_base("debug.jl", baz=baz_override, fbaz = baz_override)
                     end
WARNING: replacing module TrixiTestModule.
WARNING: could not import Main.EXAMPLES_DIR into TrixiTestModule
WARNING: could not import Main.@test_trixi_include into TrixiTestModule
════════════════════════════════════════════════════════════════════════════════════════════════════
debug.jl
baz = nothing
debug.jl: Error During Test at /home/marco/workspace/dev/TrixiTest.jl/src/macros.jl:225
  Got exception outside of a @test
  LoadError: MethodError: objects of type Nothing are not callable
  Stacktrace:
    [1] top-level scope
      @ ~/workspace/dev/TrixiTest.jl/debug.jl:7
    [2] include
      @ ./Base.jl:496 [inlined]
    [3] trixi_include(mapexpr::typeof(identity), mod::Module, elixir::String; enable_assignment_validation::Bool, replace_assignments_recursive::Bool, kwargs::@Kwargs{baz::Nothing, fbaz::Nothing})
      @ TrixiBase ~/.julia/packages/TrixiBase/MGeKl/src/trixi_include.jl:79
    [4] trixi_include
      @ ~/.julia/packages/TrixiBase/MGeKl/src/trixi_include.jl:49 [inlined]
    [5] #trixi_include#6
      @ ~/.julia/packages/TrixiBase/MGeKl/src/trixi_include.jl:86 [inlined]
    [6] trixi_include
      @ ~/.julia/packages/TrixiBase/MGeKl/src/trixi_include.jl:85 [inlined]
    [7] #8
      @ ~/workspace/dev/TrixiTest.jl/src/macros.jl:15 [inlined]
    [8] (::Base.RedirectStdStream)(thunk::Main.TrixiTestModule.var"#8#10"{Nothing}, stream::IOStream)
      @ Base ./stream.jl:1429
    [9] #7
      @ ~/workspace/dev/TrixiTest.jl/src/macros.jl:14 [inlined]
   [10] open(::Main.TrixiTestModule.var"#7#9"{Nothing}, ::String, ::Vararg{String}; kwargs::@Kwargs{})
      @ Base ./io.jl:396
   [11] open(::Function, ::String, ::String)
      @ Base ./io.jl:393
   [12] macro expansion
      @ ~/workspace/dev/TrixiTest.jl/src/macros.jl:13 [inlined]
   [13] macro expansion
      @ REPL[15]:3 [inlined]
   [14] macro expansion
      @ ~/.julia/juliaup/julia-1.10.6+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
   [15] macro expansion
      @ REPL[15]:2 [inlined]
   [16] top-level scope
      @ ~/workspace/dev/TrixiTest.jl/src/macros.jl:225
   [17] eval(m::Module, e::Any)
      @ Core ./boot.jl:385
   [18] macro expansion
      @ ~/workspace/dev/TrixiTest.jl/src/macros.jl:201 [inlined]
   [19] top-level scope
      @ REPL[15]:1
   [20] eval
      @ ./boot.jl:385 [inlined]
   [21] eval_user_input(ast::Any, backend::REPL.REPLBackend, mod::Module)
      @ REPL ~/.julia/juliaup/julia-1.10.6+0.x64.linux.gnu/share/julia/stdlib/v1.10/REPL/src/REPL.jl:150
   [22] repl_backend_loop(backend::REPL.REPLBackend, get_module::Function)
      @ REPL ~/.julia/juliaup/julia-1.10.6+0.x64.linux.gnu/share/julia/stdlib/v1.10/REPL/src/REPL.jl:246
   [23] start_repl_backend(backend::REPL.REPLBackend, consumer::Any; get_module::Function)
      @ REPL ~/.julia/juliaup/julia-1.10.6+0.x64.linux.gnu/share/julia/stdlib/v1.10/REPL/src/REPL.jl:231
   [24] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool, backend::Any)
      @ REPL ~/.julia/juliaup/julia-1.10.6+0.x64.linux.gnu/share/julia/stdlib/v1.10/REPL/src/REPL.jl:389
   [25] run_repl(repl::REPL.AbstractREPL, consumer::Any)
      @ REPL ~/.julia/juliaup/julia-1.10.6+0.x64.linux.gnu/share/julia/stdlib/v1.10/REPL/src/REPL.jl:375
   [26] (::Base.var"#1013#1015"{Bool, Bool, Bool})(REPL::Module)
      @ Base ./client.jl:437
   [27] #invokelatest#2
      @ ./essentials.jl:892 [inlined]
   [28] invokelatest
      @ ./essentials.jl:889 [inlined]
   [29] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool)
      @ Base ./client.jl:421
   [30] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:338
   [31] _start()
      @ Base ./client.jl:557
  in expression starting at /home/marco/workspace/dev/TrixiTest.jl/debug.jl:7
Test Summary: | Error  Total  Time
debug.jl      |     1      1  0.8s
ERROR: Some tests did not pass: 0 passed, 0 failed, 1 errored, 0 broken.

This is debug.jl:

baz = 1

@show baz

fbaz = () -> 1

println(fbaz())

[JoshuaLampert]

Yes, so with this PR overwriting with nothing works as expected, right? The error you posted above is expected I would say.

[MarcoArtiano]

Yes, so with this PR overwriting with nothing works as expected, right? The error you posted above is expected I would say.

Yes, exactly. The second error is indeed expected as we are trying to call the function nothing.

[vchuravy]

Isn't your comment above saying that this won't work for 1.12

[JoshuaLampert]

Not sure what exactly you mean, but $val does not work for julia v1.12 (but it works for <v1.12), but $(QuoteNode(val)) does work on julia v1.12. That is exactly why we have the @isdefined $val (true for julia <v1.12, false for julia v1.12).

[vchuravy]

3. Elixir-internal variable references (e.g. bare x=seed with no seed= key):
   on Julia >= 1.12, @isdefined returns false because bindings set inside
   Base.include have a newer world age; on older Julia the value is visible and
   also correct (same as the elixir default).

[vchuravy]

Also please /simplify the comments. Claude and co leave very verbose and often wrong comments.

[JoshuaLampert]

I am still not sure why the comment should contradict what is implemented. I find Claude's comment (I already shortened it slightly) helpful, but if you have a concrete suggestion, please make it.
If you want to know what Claude says to your question, here is its answer (sorry for copy-pasting LLM answers; I usually don't do that, but think here it's the easiest):

The reviewer is reading the comment for Case 3 — "on Julia ≥ 1.12, @isdefined returns false because bindings set inside Base.include have a newer world age" — and then seeing that line 108 uses the exact same @isdefined check for Cases 2 & 3 combined. They're asking: if @isdefined returns false on Julia ≥ 1.12, doesn't that break Case 2 (locally-defined values) as well?

The answer is no, and the reason is the distinction the comment is trying to make: @isdefined only returns false for bindings whose world age is newer than the current scope. Locally-defined values in the testset body (Case 2, like local_x = 6) are created at the same world age as the testset closure — so @isdefined local_x returns true on all Julia versions. Only bindings created inside Base.include (when trixi_include runs the elixir) are at a newer world age and therefore invisible via @isdefined on Julia ≥ 1.12.

The comment is correct but doesn't make this distinction explicit enough. It reads as if @isdefined is generally unreliable on Julia ≥ 1.12, when it's only unreliable for elixir-internal bindings. A clearer phrasing would be something like:

3. Elixir-internal variable references (e.g. bare x=seed with no seed= key):
   on Julia >= 1.12, @isdefined returns false because these bindings are created
   inside Base.include at a newer world age than the testset body; on older Julia
   the value is visible and also correct (same as the elixir default).
   Note: locally-defined values (case 2) are at the same world age as the testset,
   so @isdefined correctly returns true for them on all Julia versions.

Does this answer your question? If yes, should we adjust the comment for clarification, should I reduce the comment further (as you suggested), or is it fine? If not, could you please explain more your concerns, ideally by making suggestions to improve the comment or asking for concrete examples I can test?

[ranocha]

Thanks. I talked with Valentin offline. We decided that this can lead to surprises since the usual scoping rules of Julia are violated. However, implemeting scoping rules here is not an option, so I added a few more tests to kind of documented the currently expected behavior.