Skip to content

Conversation

zrphercule
Copy link
Contributor

The second part of T32009899

@ezyang
Copy link
Contributor

ezyang commented Sep 12, 2018

For ease of reviewing, can we avoid moving the location of these files? It thwarts diff.

@zrphercule
Copy link
Contributor Author

For ease of reviewing, can we avoid moving the location of these files? It thwarts diff.

Oh sure, I thought it is better to have a seperate repo for gtest at beginning, but actually it doesnt matter

@zrphercule zrphercule changed the title [Dont Review]Migrate test in cpp/api/ to use gtest Migrate test in cpp/api/ to use gtest Sep 14, 2018
for (size_t i = 0; i < v.size(); ++i) {
REQUIRE(v.at(i) == 1 + i);
}
// TEST_CASE("static") {

This comment was marked as off-topic.

REQUIRE(v.at(i) == 1 + i);
}
// TEST_CASE("static") {
TEST(static, static){

This comment was marked as off-topic.

@@ -471,19 +472,36 @@ if (BUILD_TEST AND NOT NO_API AND NOT USE_ROCM)

target_link_libraries(test_api torch ${TORCH_CUDA_LIBRARIES} ${CUDA_NVRTC_LIB} ${CUDA_CUDA_LIB})

#Google test of api.
set(TORCH_API_GTEST_DIR "${TORCH_ROOT}/test/cpp/api/")

This comment was marked as off-topic.

This comment was marked as off-topic.

add_executable(gtest_api
${TORCH_API_GTEST_DIR}/static.cpp
)
target_include_directories(gtest_api PUBLIC ${ATen_CPU_INCLUDE})

This comment was marked as off-topic.

if (NOT MSVC)
if (APPLE)
target_compile_options(test_api PRIVATE
# Clang has an unfixed bug leading to spurious missing braces
# warnings, see https://bugs.llvm.org/show_bug.cgi?id=21629
-Wno-missing-braces)
else()
target_compile_options(gtest_api PRIVATE

This comment was marked as off-topic.

@goldsborough
Copy link
Contributor

Thanks for working on this! It's a good start. A couple things need to be improved.

@zrphercule
Copy link
Contributor Author

@goldsborough Any more comments? =)

Copy link
Contributor

@goldsborough goldsborough left a comment

Choose a reason for hiding this comment

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

Cool, let's merge this asap

REQUIRE(v.at(i) == 1 + i);
}
TEST(TestStatic, All_Of){
EXPECT_EQ(torch::all_of<>::value, true);

This comment was marked as off-topic.

This comment was marked as off-topic.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

zrphercule is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@zrphercule zrphercule deleted the migrate_test branch September 18, 2018 18:11
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants