serac icon indicating copy to clipboard operation
serac copied to clipboard

Simplify Dependencies

Open chapman39 opened this issue 3 years ago • 3 comments

Fixes #788

Here's a before-and-after of one of the graphs: Screen Shot 2022-10-14 at 2 48 42 PM Screen Shot 2022-10-14 at 2 48 54 PM

The only unneeded connections are with gtest. I tried making test_utils depend on gtest_main instead, but that gave me a bunch of gtest errors, so I decided not to mess with it.

serac_functional also has some private dependencies. If they were public these graphs could be simplified further (for example in the pic above serac_physics_integrators->serac_numerics could be removed), but I assumed they were private for a reason.

I've also created a python script (generate_images.py) that generates a bunch of images of the dependency graphs. It might be useful, but if not I can remove it from this PR. If you want me to keep it in I can add documentation for it.

Note this doesn't re-organize the tests it just removes the unnecessary dependencies.

chapman39 avatar Oct 07 '22 00:10 chapman39

Codecov Report

Merging #820 (ab96efa) into develop (7a6ef7a) will not change coverage. The diff coverage is n/a.

:exclamation: Current head ab96efa differs from pull request most recent head b6d772b. Consider uploading reports for the commit b6d772b to get more accurate results

@@           Coverage Diff            @@
##           develop     #820   +/-   ##
========================================
  Coverage    95.00%   95.00%           
========================================
  Files          143      143           
  Lines         9372     9372           
========================================
  Hits          8904     8904           
  Misses         468      468           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Oct 11 '22 01:10 codecov-commenter

Note this doesn't re-organize the tests it just removes the unnecessary dependencies.

This PR is a step in the right direction, but until the tests are reorganized, we won't be able to remove most of their dependencies (but this can be done in a later PR).

For example, serac/infrastructure/tests/serac_error_handling.cpp has a test TEST(SeracErrorHandling, BcRetrieveScalarCoef) that depends on BoundaryCondition, which is not part of serac/infrastructure. So, in order for it to compile, we broadly made all the tests in serac/infrastructure depend on serac_physics to resolve BoundaryCondition.

To me, it makes more sense to move that test to serac/physics/tests or serac/physics/boundary_conditions/tests, rather than serac/infrastructure/tests. Doing this would make it so that the dependencies of serac/infrastructure/tests will be smaller.


Another way of putting it: I think our test directories should all have a CMakeLists.txt that has the following pattern (using serac/infrastructure/tests/CMakeLists.txt as an example):

# note: the tests should only depend on the parent directory target and gtest
set(infrastructure_test_dependencies gtest serac_infrastructure)

and if a test like serac_error_handling.cpp doesn't compile because it needs to know about serac_physics, the solution should not be to expand infrastructure_test_dependencies to include all of serac, but to just move the test (either the whole file, or just the actual gtest function) to a different location in the project that has the appropriate dependencies.

samuelpmishLLNL avatar Oct 14 '22 23:10 samuelpmishLLNL

@samuelpmishLLNL yeah that makes sense. we can try and get this pr in and in a separate pr i could start re-organizing the tests. i can remove the "fixes 788" thing for now since this doesn't resolve everything in the issue.

chapman39 avatar Oct 15 '22 00:10 chapman39