Skip to content
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

build: Removed GTest::gtest_main from CMakeLists.txt for velox_simple_aggregate_test #11668

Conversation

minhancao
Copy link
Contributor

@minhancao minhancao commented Nov 26, 2024

Removed GTest::gtest_main from CMakeLists.txt for velox_simple_aggregate_test.

add_executable(velox_simple_aggregate_test SimpleAggregateAdapterTest.cpp
                                           Main.cpp)

Since Main.cpp is already included here, GTest::gtest_main is not needed in target_link_libraries.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 26, 2024
Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit bc05309
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6748d575a54a3f0008c7ef45

@assignUser assignUser changed the title fix: Removed GTest::gtest_main from CMakeLists.txt for velox_simple_aggregate_test build: Removed GTest::gtest_main from CMakeLists.txt for velox_simple_aggregate_test Nov 27, 2024
Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

This is incidental to your actual change but that test isn't run in CI because ctest doesn't know about it. Could you add an add_test for it so that it's picked up?

@minhancao minhancao force-pushed the remove_gtest_main_for_velox_simple_aggregate_test branch from 84664ee to eece5ba Compare November 27, 2024 18:24
@minhancao minhancao force-pushed the remove_gtest_main_for_velox_simple_aggregate_test branch from eece5ba to e2b4375 Compare November 27, 2024 19:20
Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

@assignUser assignUser added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 28, 2024
@assignUser
Copy link
Collaborator

velox_exec_test was failing on main the sameway. As the actually modified test is working fine I see no reason to re-run the job but a rebase should fix it.

@minhancao minhancao force-pushed the remove_gtest_main_for_velox_simple_aggregate_test branch from e2b4375 to bc05309 Compare November 28, 2024 20:41
@facebook-github-bot
Copy link
Contributor

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng merged this pull request in 4dd6499.

TongWei1105 pushed a commit to TongWei1105/velox that referenced this pull request Dec 21, 2024
…_aggregate_test (facebookincubator#11668)

Summary:
Removed GTest::gtest_main from CMakeLists.txt for velox_simple_aggregate_test.

```
add_executable(velox_simple_aggregate_test SimpleAggregateAdapterTest.cpp
                                           Main.cpp)
```

Since Main.cpp is already included here, GTest::gtest_main is not needed in target_link_libraries.

Pull Request resolved: facebookincubator#11668

Reviewed By: kagamiori

Differential Revision: D66690701

Pulled By: xiaoxmeng

fbshipit-source-id: 51c5536de45bfd15abdf7a83f46c95f09846a976
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
…_aggregate_test (facebookincubator#11668)

Summary:
Removed GTest::gtest_main from CMakeLists.txt for velox_simple_aggregate_test.

```
add_executable(velox_simple_aggregate_test SimpleAggregateAdapterTest.cpp
                                           Main.cpp)
```

Since Main.cpp is already included here, GTest::gtest_main is not needed in target_link_libraries.

Pull Request resolved: facebookincubator#11668

Reviewed By: kagamiori

Differential Revision: D66690701

Pulled By: xiaoxmeng

fbshipit-source-id: 51c5536de45bfd15abdf7a83f46c95f09846a976
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants