Remove coverage_override and use primary test run for code coverage

[vchuravy]

Julia has improved it's coverage handling #1075 (comment)

Let's test and see if we still need this complication.

[github-actions]

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

• The PR has a single goal that is clear from the PR title and/or description.
• All code changes represent a single set of modifications that logically belong together.
• No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

• The code can be understood easily.
• Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
• There are no redundancies that can be removed by simple modularization/refactoring.
• There are no leftover debug statements or commented code sections.
• The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

• New functions and types are documented with a docstring or top-level comment.
• Relevant publications are referenced in docstrings (see example for formatting).
• Inline comments are used to document longer or unusual code sections.
• Comments describe intent ("why?") and not just functionality ("what?").
• If the PR introduces a significant change or new feature, it is documented in NEWS.md with its PR number.

Testing

• The PR passes all tests.
• New or modified lines of code are covered by tests.
• New or modified tests run in less then 10 seconds.

Performance

• There are no type instabilities or memory allocations in performance-critical parts.
• If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

• The correctness of the code was verified using appropriate tests.
• If new equations/methods are added, a convergence test has been run and the results
  are posted in the PR.

Created with ❤️ by the Trixi.jl community.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.87%. Comparing base (81d8e47) to head (b911210).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2254      +/-   ##
==========================================
+ Coverage   96.42%   96.87%   +0.45%     
==========================================
  Files         490      490              
  Lines       39426    39427       +1     
==========================================
+ Hits        38015    38194     +179     
+ Misses       1411     1233     -178     

Flag	Coverage Δ
unittests	96.87% <ø> (+0.45%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[vchuravy]

Looks like this makes CI faster:

• Total compute time 748m -> 438m
• Peak: 104m to 39m

[JoshuaLampert]

LGTM! However, I would also like to see green light from @ranocha.
This is a huge improvement in CI time. Really nice to see.

[ranocha]

Thanks a lot! This looks already quite nice. Could you please fix the tests using skip_coverage? Since we do not run without coverage reports anymore, these tests will be skipped completely - which we should not do.

[vchuravy]

The remaining special logic is

Trixi.jl/test/runtests.jl

Lines 12 to 47 in 1b21e62

	@time @testset "Trixi.jl tests" begin
	# This is placed first since tests error out otherwise if `TRIXI_TEST == "all"`,
	# at least on some systems.
	@time if TRIXI_TEST == "all" || TRIXI_TEST == "mpi"
	# Do a dummy `@test true`:
	# If the process errors out the testset would error out as well,
	# cf. https://github.com/JuliaParallel/MPI.jl/pull/391
	@test true
	# There are spurious test failures of Trixi.jl with MPI on Windows, see
	# https://github.com/trixi-framework/Trixi.jl/issues/901
	# To reduce their impact, we do not test MPI with coverage on Windows.
	# This reduces the chance to hit a spurious test failure by one half.
	# In addition, it looks like the Linux GitHub runners run out of memory during the 3D tests
	# with coverage, so we currently do not test MPI with coverage on Linux. For more details,
	# see the discussion at https://github.com/trixi-framework/Trixi.jl/pull/1062#issuecomment-1035901020
	cmd = string(Base.julia_cmd())
	coverage = occursin("--code-coverage", cmd) &&
	!occursin("--code-coverage=none", cmd)
	if !(coverage && Sys.iswindows()) && !(coverage && Sys.isapple())
	# We provide a `--heap-size-hint` to avoid/reduce out-of-memory errors during CI testing
	mpiexec() do cmd
	run(`$cmd -n $TRIXI_MPI_NPROCS $(Base.julia_cmd()) --threads=1 --check-bounds=yes --heap-size-hint=0.5G $(abspath("test_mpi.jl"))`)
	end
	end
	end
	@time if TRIXI_TEST == "all" || TRIXI_TEST == "threaded" ||
	TRIXI_TEST == "threaded_legacy"
	# Do a dummy `@test true`:
	# If the process errors out the testset would error out as well,
	# cf. https://github.com/JuliaParallel/MPI.jl/pull/391
	@test true
	run(`$(Base.julia_cmd()) --threads=$TRIXI_NTHREADS --check-bounds=yes --code-coverage=none $(abspath("test_threaded.jl"))`)
	end

Unsure what to do there.

After talking to @ranocha offline. We should try to enable this again.

[vchuravy]

@bennibolm could I ask you to look at the "sc subcell" errors.

@ranocha and my working hypothesis is that code coverage impact the code generated by Julia and thus it can prevent optimizations to trigger like the formation of fma operations.

Fixed.

[vchuravy]

@bennibolm elixir_euler_supersonic_cylinder_sc_subcell in test_p4est_2d.jl:504

has:

julia> -(0.11085870166618325, 0.11085869686488965)
4.801293598810652e-9

julia> -(0.23309905989870722, 0.23309903629883755)
2.359986966959937e-8

julia> -(0.7932047512585592, 0.7932047196189057)
3.1639653452764094e-8

[bennibolm]

@bennibolm elixir_euler_supersonic_cylinder_sc_subcell in test_p4est_2d.jl:504

has:

julia> -(0.11085870166618325, 0.11085869686488965)
4.801293598810652e-9

julia> -(0.23309905989870722, 0.23309903629883755)
2.359986966959937e-8

julia> -(0.7932047512585592, 0.7932047196189057)
3.1639653452764094e-8

Yes, I already looked at it. Normally, I would easily say, no worries, subcell limiting is built to be very susceptible to small changes. Even with those differences. What makes a little bit cautious is that I never really had those issues with this particular elixir (And normally there were always the same which caused errors).
In its defense, it is relatively new and was only merged at the end of August. So, maybe it was just not that often exposed to updates. What do you think @amrueda ? I would still be okay with just updating the errors.

[ranocha]

@vchuravy The CI failure

TreeMesh1D: elixir_navierstokes_convergence_walls_amr.jl: GradientVariablesEntropy: Test Failed at /home/runner/work/Trixi.jl/Trixi.jl/test/test_trixi.jl:77
  Expression: isapprox(linf_expected, linf_actual, atol = 1.0e-9, rtol = 1.4901161193847656e-8)
   Evaluated: isapprox(0.0009597461727750556, 0.0009597423024825247; atol = 1.0e-9, rtol = 1.4901161193847656e-8)

(see https://github.com/trixi-framework/Trixi.jl/actions/runs/13139594649/job/36663263375?pr=2254#step:7:3207) should be acceptable from my point of view - just change the value. It seems to be susceptible to changes since we have already a relatively loose tolerance.

julia> -(0.0009597461727750556, 0.0009597423024825247)
3.870292530905317e-9

[ranocha]

Almost ready 👍

[vchuravy]

Total improvement is 1h 16m to 35m

[amrueda]

@bennibolm elixir_euler_supersonic_cylinder_sc_subcell in test_p4est_2d.jl:504
has:

julia> -(0.11085870166618325, 0.11085869686488965)
4.801293598810652e-9

julia> -(0.23309905989870722, 0.23309903629883755)
2.359986966959937e-8

julia> -(0.7932047512585592, 0.7932047196189057)
3.1639653452764094e-8

Yes, I already looked at it. Normally, I would easily say, no worries, subcell limiting is built to be very susceptible to small changes. Even with those differences. What makes a little bit cautious is that I never really had those issues with this particular elixir (And normally there were always the same which caused errors). In its defense, it is relatively new and was only merged at the end of August. So, maybe it was just not that often exposed to updates. What do you think @amrueda ? I would still be okay with just updating the errors.

I find this a bit weird because this PR is only changing how coverage is tested, right?... So nothing is really changing in the initial condition or in the numerics... right?! 🤔

[ranocha]

Thanks a lot!

[vchuravy]

or in the numerics... right?!

The emission of coverage impacts the generated LLVM code and can inhibit code transformations (like fma, vectorization etc)