[newchem-cpp] Improve API-Test Harness (1/2): Chemistry Parameters#499
Open
mabruzzo wants to merge 26 commits intograckle-project:newchem-cppfrom
Open
[newchem-cpp] Improve API-Test Harness (1/2): Chemistry Parameters#499mabruzzo wants to merge 26 commits intograckle-project:newchem-cppfrom
mabruzzo wants to merge 26 commits intograckle-project:newchem-cppfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To be reviewed after #468 has been merged
This PR is dedicated to improving the test Harness for using Googletest to execute API functions.
This PR specifically modifies the way that we work with the
grtest::GrackleCtxPacktype.Background Context
The
grtest::GrackleCtxPacktype was introduced in a prior PR. It is VERY similar to gracklepy’schemistry_datapython extension type in that it tracks instances of Grackle’schemistry_dataandchemistry_data_storagetypes. It also tracks the initial ‘code_unitsstruct used to initializechemistry_data_storage`.This type essentially serves 2 purposes:
Description
This PR changes a few related things.
grtest::GrackleCtxPackgrackle_data_file) in thechemistry_datainstance wrapped bygrtest::GrackleCtxPackessentially had to hold a string literal (the alternative was quite clunky)chemistry_datainstance wrapped bygrtest::GrackleCtxPackto hold arbitrary strings (very useful for testing purposes!)grtest::ParamValtype:grtest::ParamValis type-safe wrapper around a union that can hold any kind of parameter value used by Grackle (i.e. a string, a nullptr, an int, a double)grtest::ParamValinstance)grtest::ParamValchemistry_data. Now, they specify a vector of name-grtest::ParamValpairs that are subsequently used to initializechemistry_data. This makes it easy to override/modify optionsgrtest::StatustypeOverall, this make the writing of api-tests with the googletest framework much more convenient. Things are still a little clunky, but I want to hold off on more refactoring until after we have written some tests
Payoff
The payoff of the changes in this PR are probably not immediately obvious. But they will become more obvious in some followup PRs that I will link here that will introduce some new tests to the Googletest framework.
I also have some ambitious ideas to reuse this machinery: