Skip to content

Convert cxx_grid_example to a googletest test-case#254

Merged
brittonsmith merged 6 commits into
grackle-project:mainfrom
mabruzzo:convert-cxx_grid_example_to_gtest
Mar 25, 2025
Merged

Convert cxx_grid_example to a googletest test-case#254
brittonsmith merged 6 commits into
grackle-project:mainfrom
mabruzzo:convert-cxx_grid_example_to_gtest

Conversation

@mabruzzo

@mabruzzo mabruzzo commented Feb 9, 2025

Copy link
Copy Markdown
Collaborator

This PR converts the cxx_grid_example.C "code example" into a test run as part of the googletest suite

Some background:

  • I originally wrote the cxx_grid_example.C file back when I fixed the logic in some of the calculate_<quan> functions in order to respect the grid_start and grid_end members of the grackle_field_data type.
  • This was never really a "code example."
    • It was really just a test that all of the routines would respect grid_start and grid_end. However, this test could not be part of the pytest testing framework, since the python bindings hard-code the values of grid_start and grid_end.
    • Treating it like a "code example" was the easiest way to make sure that the logic always got run as part of automated testing

In any case, I think it obviously makes a lot of sense to shift this test to the googletest framework. This is easiest to review commit-by-commit:

  1. The 1st commit did the minimal amount of work to get the logic wired into the googletest testing framework. The test runs, but will produce memory leaks if the test fails.
  2. The 2nd commit factored out some functionality that we will probably want to reuse in some other tests. The primary machinery relates to locating the grackle data file.1 It also introduces some machinery for portable and reproducible random number sampling.2
  3. The third and fourth commits refactor some test-logic:
    • I factored out most grackle-struct setup logic into a separate function (since it has little to do with the test's core logic)
    • I removed the for-loop over the primordial_chemistry value and converted the test so it was parameterized by the primordial_chemistry value (in other words, we now run 4 separate test cases)
    • I tweaked some logic around our management of the grackle_field_data struct to reduce memory leaks on test-failure
    • I moved the setup and tear-down of grackle structs into the test fixture to make sure there are no memory leaks when the test fails (I don't love doing moving logic into fixture, but it was the easiest way to accomplish this goal)
    • I modified how errors are reported to use the mechanism understood by googletest

This was all a little more involved than I expected (I thought it would take under an hour), so I decided to cut my losses at this point. There are a few more tweaks I think would be good for the future:

  • shift to using local data structures (from global data structures)
  • reuse the test fixtures and helper functions to add tests that:

Footnotes

  1. The logic for doing this is a little hacky. Currently, PR Modern Version Of: Transcribe contents of interpolators_g.F from Fortran to C #251 crudely implements similar logic its own tests. The idea here is to provide an abstraction for this hacky logic that can be reused across multiple tests. If PRs Auto File Management Part 1: Introducing a Datafile Management Tool #235, Auto File Management Part 2: Allow Grackle to search for automatically managed data files #237, and Auto File Management Part 3: Install data-management tool as command line program #246 get merged, then we can refactor the abstraction

  2. in reality, we don't actually need this function right now. The test_ghost_zone logic is just looking for arbitrary values for the sake of "finger-printing" (we are just trying to confirm that the randomized arbitrary values are unchanged -- the values shouldn't actually affect the output values). I only realized this after I wrote up a long comment justifying its existence. Since this will inevitably be useful in the future, we're going to hold onto the logic.
    [^3] like grid_start[i] >= grid_dimension[i], grid_end[i] >= grid_dimension[i], grid_start[i] > grid_end[i], grid_start[i] < 0, grid_end[i] < 0, grid_dimension[i] < 0,

The cxx_grid_example was never really meant to be an example. In reality
it was a crude way of writing some test-code that could not be directly
implemented in pytest.

The new test_ghost_zone file could use a little refactoring
The idea is to factor out reusable testing-logic into this library. At
present, we have factored out logic to:
- set the value of `chemistry_data_file::grackle_data_file`. This is
  actually really important logic (it deals with figuring out the path
  to the data directory) and would justify the existence of
  the library on its own
- drawing a random uniformly distributed value on the unit interval in
  portable, reproducible manner
  - in reality, we don't actually need this function right now. The
    test_ghost_zone logic is just looking for arbitrary values for
    the sake of "finger-printing" (we are just trying to confirm that
    the randomized arbitrary values are unchanged). I only realized
    this after I wrote up a long comment justifying its existence.
    Since this will inevitably be useful in the future, we're going
    to hold onto the logic.
@mabruzzo mabruzzo added the testing test suite, regression tests, ci infrastructure label Feb 28, 2025

@brittonsmith brittonsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this in PR #258.

@brittonsmith brittonsmith merged commit c1dd468 into grackle-project:main Mar 25, 2025
@mabruzzo mabruzzo deleted the convert-cxx_grid_example_to_gtest branch March 26, 2025 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing test suite, regression tests, ci infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants