Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reorganize tests #809

Merged
merged 24 commits into from
Aug 25, 2021
Merged

reorganize tests #809

merged 24 commits into from
Aug 25, 2021

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Aug 20, 2021

TODO:

  • On macOS/Windows, only run MPI tests, threaded tests, and exercise important or “fragile” binary dependencies (like P4est)
    • Keep 2d_mpi
    • update 2d_threaded
  • Decide what to do with misc
  • Distribute parts approximately uniformly (by run time)
  • Update required status checks

Closes #673; closes #794

@sloede
Copy link
Member

sloede commented Aug 24, 2021

The following consensus was reached at today's Trixi meeting:

  • On macOS/Windows, only run MPI tests, threaded tests, and exercise important or “fragile” binary dependencies (like P4est)
  • Use only Linux tests for coverage reports to speed up the process of adding new tests
  • Try to configure CI to run macOS/Windows tests only on ready-for-review PRs to increase turnover and reduce pressure on concurrent jobs when still in draft phase. This can (maybe) be achieved by looking at the github.event.pull_request.draft == true/false property

@ranocha
Copy link
Member Author

ranocha commented Aug 24, 2021

  • Try to configure CI to run macOS/Windows tests only on ready-for-review PRs to increase turnover and reduce pressure on concurrent jobs when still in draft phase. This can (maybe) be achieved by looking at the github.event.pull_request.draft == true/false property

Could you please open a new issue with this?

@ranocha ranocha marked this pull request as ready for review August 25, 2021 08:54
@ranocha ranocha requested a review from sloede August 25, 2021 08:54
@ranocha
Copy link
Member Author

ranocha commented Aug 25, 2021

We need to update the required status checks before merging this.

@ranocha ranocha changed the title WIP: reorganize tests reorganize tests Aug 25, 2021
@sloede
Copy link
Member

sloede commented Aug 25, 2021

While going through it, I realize that you removed many RestartCallbacks. Are all mesh-solver combinations still checked for restarting properly? And what was your motivation (just curious)?

@ranocha
Copy link
Member Author

ranocha commented Aug 25, 2021

While going through it, I realize that you removed many RestartCallbacks. Are all mesh-solver combinations still checked for restarting properly?

They should be - I tried to include at least one example using a restart callback.

And what was your motivation (just curious)?

The restart callbacks

  • are just annoying locally in many elixirs since they produce unnecessary IO
  • take CI time that we can save easily

@ranocha
Copy link
Member Author

ranocha commented Aug 25, 2021

We might think about merging threaded and mpi jobs on each OS

Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, with one proposal for more consistency (of course 😎) and the request to not remove the LSA ICs/BCs, plus a few minor questions/remarks.

test/runtests.jl Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
test/test_special_elixirs.jl Show resolved Hide resolved
test/test_special_elixirs.jl Show resolved Hide resolved
test/test_tree_3d_part2.jl Show resolved Hide resolved
test/test_unit.jl Show resolved Hide resolved
src/equations/linear_scalar_advection_1d.jl Outdated Show resolved Hide resolved
@sloede
Copy link
Member

sloede commented Aug 25, 2021

One thing I forgot: I think it would be good to adapt the relevant section in the docs as well. That is, fix the obsolete file name and maybe add a sentence or two about where to add new tests, such that everything remains roughly balanced?

@ranocha
Copy link
Member Author

ranocha commented Aug 25, 2021

One thing I forgot: I think it would be good to adapt the relevant section in the docs as well. That is, fix the obsolete file name and maybe add a sentence or two about where to add new tests, such that everything remains roughly balanced?

Good point - I'll do that

@ranocha ranocha requested a review from sloede August 25, 2021 12:40
sloede
sloede previously approved these changes Aug 25, 2021
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for taking care of this mundane but extremely crucial piece of infrastructure work!

@ranocha ranocha enabled auto-merge (squash) August 25, 2021 13:00
@ranocha ranocha requested a review from sloede August 25, 2021 13:21
@ranocha ranocha closed this Aug 25, 2021
auto-merge was automatically disabled August 25, 2021 13:21

Pull request was closed

@ranocha ranocha reopened this Aug 25, 2021
@ranocha ranocha enabled auto-merge (squash) August 25, 2021 13:21
.codecov.yml Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #809 (6158da9) into main (6491e0c) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #809      +/-   ##
==========================================
- Coverage   92.75%   92.72%   -0.02%     
==========================================
  Files         188      188              
  Lines       17398    17399       +1     
==========================================
- Hits        16136    16133       -3     
- Misses       1262     1266       +4     
Flag Coverage Δ
unittests 92.72% <100.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...dgsem/elixir_advection_amr_solution_independent.jl 100.00% <ø> (ø)
...xamples/p4est_3d_dgsem/elixir_euler_free_stream.jl 100.00% <ø> (ø)
...s/structured_3d_dgsem/elixir_euler_source_terms.jl 100.00% <ø> (ø)
...ree_2d_dgsem/elixir_advection_amr_coarsen_twice.jl 100.00% <ø> (ø)
...tree_2d_dgsem/elixir_advection_amr_refine_twice.jl 100.00% <ø> (ø)
...dgsem/elixir_advection_amr_solution_independent.jl 100.00% <ø> (ø)
...ree_2d_dgsem/elixir_advection_amr_visualization.jl 100.00% <ø> (ø)
examples/tree_2d_dgsem/elixir_lbm_couette.jl 80.00% <ø> (ø)
src/solvers/dgsem_p4est/dg_2d.jl 99.02% <100.00%> (+<0.01%) ⬆️
src/callbacks_step/save_restart.jl 84.00% <0.00%> (-8.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6491e0c...6158da9. Read the comment docs.

@ranocha ranocha disabled auto-merge August 25, 2021 15:04
@ranocha ranocha merged commit 5f1324d into main Aug 25, 2021
@ranocha ranocha deleted the hr/tests branch August 25, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce test duration in misc CI job or split into multiple jobs Reorganize tests
2 participants