Skip to content

Conversation

@mabruzzo
Copy link
Collaborator

@mabruzzo mabruzzo commented May 17, 2025

This PR moves move test_chemistry_struct_synched between test suites (from pygrackle-suite to the corelib-suite).

At a high-level, this makes sense because this test deals with source code files.

Costs

This introduces some more lines of code. I could probably cut down on some of it (I reused some generic code I had written for a personal project, so it's a little more generic than necessary).

Benefits

This change has 3 advantages:

  1. running the test_chemistry_struct_synched test in the pygrackle-suite, it severely limited the organization of the public grackle headers. When the test was invoked from the pygrackle-suite, the header read in by the castxml tool (to probe the chemistry_data type) could not have a transitive dependence on the grackle_float.h generated header (because the pygrackle tests can't make any assumptions about the existence/location of a build-directory).

    • This has been inconvenient in the past (e.g. in PR Introduce gr_required_units #209)
    • by moving the test between suites, I was able to pass the grackle.h header to the castxml tool (rather than grackle_chemistry_data.h, which raises warnings when it is read directly), and this test is no longer a barrier to reorganizing public headers.
  2. it opens the door to reuse the machinery to write additional tests for other structs. For example:

    • we could test consistency of the XMACROS with grackle_field_data
    • an argument could be made for testing accessibility of members in the chemistry_data_storage struct (e.g. for the sort of API introduced within PR [newchem-cpp] rate access api #316
    • there are a number of internal types introduced within the newchem-cpp transcription PRs that implement a kind of "manual-reflection." This machinery is generally important for managing memory and will be useful for porting to GPUs. It would be extremely useful if we could reuse this testing-machinery to ensure that the "manual-reflection" includes all fields of the structs.

    While it is theoretically possible to test these scenarios through pytest, it would involve writing a bunch of extra specialized additional C & Cython code for each case (more than would be necessary for testing stuff through GTest). Furthermore, testing the internal types with Cython would mean that Cython needs access to internal headers (which I don't think we necessarily want).

  3. Less importantly, because the build-system now invokes castxml whenever the test-cases are built, the struct definition and the logic in the test case are always guaranteed to be synchronized with each other (this was not always the case)

Other Thoughts

In the future, we could replace the usage of castxml with a C++ library like https://github.com/boostorg/pfr (this doesn't need the rest of boost) or https://github.com/stephenberry/glaze. Both of these libraries would be easy to auto-install through CMake for this test (just like googletest) and could be used to automatically infer the list of members in a struct. The one "catch" is we would need to require C++20/C++23 for this particular test (which wouldn't be a huge deal)

mabruzzo added 4 commits May 17, 2025 10:31
This change was made in anticipation of shifting the
"test_grackle_chemistry_field_synched" test from the pygrackle
test-suite to the core-lib test suite.

In more detail, the test doesn't really belong in the pygrackle
test-suite since it is looking at header files. As currently written,
the test current places a strong constraint on the way in which we
organize our public headers (it currently can't read a header that
depends on an autogenerated header file). Shifting this test to the
core-lib test suite will remove this limitation
This introduces some more lines of code. I could probably cut down on
some of it (I reused some generic code I had writen for a personal
project, so it's a little more generic than necessary).

This change has 2 major advantages:

1. with the test_chemistry_struct_synched test in the pygrackle
   test-suite, it severely limits the organization of the public grackle
   headers. When the test was invoked from the pygrackle-framework, it
   meant that the header containing the declaration of the
   chemistry_data could not have a transative dependence on the
   grackle_float.h generated header (because the pygrackle tests can't
   make any assumptions about the existence/location of a
   build-directory). This has been inconvenient in the past.

2. it opens the door to reuse the machinery to write additional tests
   for other structs. For example:
   - we could test consistency of the XMACROS with grackle_field_data
   - an argument could be made for testing accessibility of members in
     the chemistry_data_storage struct (e.g. for the sort of API
     introduced within PR grackle-project#316
   - there are a number of internal types introduced within the
     newchem-cpp transcription PRs that implement a kind of
     "manual-reflection." This machinery is generally important for
     managing memory and will be useful for porting to GPUs. It would be
     extremely useful if we could reuse this testing-machinery to ensure
     that the "manual-reflection" includes all fields of the structs.

   While it is theoretically possible to test these scenarios through
   pytest, it would involve writing a bunch of extra specialized
   additional C & Cython code for each case (more than would be
   necessary for testing stuff through GTest). Furthermore, testing the
   internal types with Cython would mean that Cython needs access to
   internal headers (which I don't think we necessarily want).

> [!NOTE]
> A compelling case could be made that we should be invoking the castxml
> tool & python script when compiling the tests. This (i) trades ~300
> lines of C++ for some CMake logic and (ii) it means that the tests
> will be better behaved if the source-code is modified between
> compilation & running the test. But, I only really considered that
> when I was mostly done (and IMO, this change is already a significant
> improvement)
@mabruzzo mabruzzo added the testing test suite, regression tests, ci infrastructure label May 17, 2025
Copy link
Contributor

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

I'm mostly going to take your word for it on this one.

@brittonsmith brittonsmith merged commit bd85de4 into grackle-project:main May 26, 2025
5 checks passed
@mabruzzo mabruzzo deleted the chemistry_data_member_test_modify branch June 9, 2025 12:50
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