-
Notifications
You must be signed in to change notification settings - Fork 51
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
Example Google Test created #243
base: main
Are you sure you want to change the base?
Example Google Test created #243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic! Thanks for making this PR! I'm really excited to start merging your work into Grackle.
Before I approve the PR, I wanted to request a couple changes -- these all pertain to very minor things in the CMakeLists.txt files (the tests themselves look great!). Most of the suggestions are stylistic. (although one or two of them actually fixes some bugs that I had encountered when I tried to compile/run the tests on my machine). It goes without saying, but you should obviously feel free to push on any suggestions that you disagree with.
It would also be great if we could run these tests as part of our continuous integration. I would be happy to make the changes myself (and push the changes to your branch) or you could make the changes yourself (just make the change described in the drop-down at the end of this message.
Detailed description of modifying continuous integration
The quickest way to do this would be to replace the lines 308 to 314, which currently look like
- run-tests:
omp: 'false'
build_kind: 'cmake'
generate: 'false'
# TODO: once we merge in GH-#208, we need to issue a new gold standard, and then we should
# issue a followup PR where we uncomment the remainder of this test
with the following chunk. In practice, we aren't actually deleting anything (we are just inserting some new lines). As you almost certainly know, whitespace is obviously important here.
- run-tests:
omp: 'false'
build_kind: 'cmake'
generate: 'false'
- run:
name: "Execute the C++ tests"
command: |
# compile the tests (it would probably would be better to do this when
# compiling the rest of Grackle)
cmake -DGRACKLE_BUILD_TESTS=ON -Bbuild
cmake --build build
# execute the tests
cd build
ctest
# TODO: once we merge in GH-#208, we need to issue a new gold standard, and then we should
# issue a followup PR where we uncomment the remainder of this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making all of the changes! Could you make 3 other minor tweaks for me?
- Only 1 of them is strictly necessary (see the comment where I suggest reintroducing the
include(setup_GTest)
andadd_subdirectory(tests)
line). After these are fixed the unit tests will actually be run by CI - the other 2 comments aren't strictly necessary.
Otherwise this looks great! I'm hitting approve now. After you make the requested change(s), I think this will be ready to merge (you should be able to hit GitHub's commit suggestion" button for each of my suggested changes).
(I'm not sure if @brittonsmith also wants to sign off on this before merging)
I think this is ready to be merged. @brittonsmith do you want to take a look first? Or should we go ahead and merge? FYI It should NOT produce any conflicts for PR #177 |
Hi @mabruzzo, this PR contains the example of Google Test integration (interpolator unit tests) to Grackle main branch we talked about during our latest call.