Changed from google test to catch #961

teseoch · 2018-10-19T22:07:14Z

Still missing to port INSTANTIATE_TEST_CASE_P

Check all that apply (change to `[x]`)

All changes meet libigl style-guidelines.
Adds new .cpp file.
Adds corresponding unit test.
Adds corresponding python binding.
This is a minor change.

alecjacobson

This is great. We can't merge until all tests are ported.

jdumas · 2018-10-19T22:22:13Z

tests/CMakeLists.txt


 SET(TEST_ROOT_DIR ${CMAKE_CURRENT_LIST_DIR})
+list(APPEND CMAKE_MODULE_PATH ${LIBIGL_EXTERNAL}/Catch2/contrib)
 INCLUDE_DIRECTORIES(${TEST_ROOT_DIR})


I think the TEST_ROOT_DIR and the include_directory can go away now. Or at least should use an interface target like the tutorials.

jdumas · 2018-10-19T22:24:37Z

tests/CMakeLists.txt


 # Process code in each subdirectories: add in decreasing order of complexity
 # (last added will run first and those should be the fastest tests)
 IF(LIBIGL_WITH_MOSEK)
-  ADD_SUBDIRECTORY(${TEST_ROOT_DIR}/include/igl/mosek)
+  file(GLOB TEST_SRC_FILES ${TEST_ROOT_DIR}/include/igl/mosek/*.cpp)


${TEST_ROOT_DIR}/ is just . in this case, can be removed.

jdumas · 2018-10-19T22:25:09Z

tests/CMakeLists.txt

+  file(GLOB TEST_INC_FILES ${TEST_ROOT_DIR}/include/igl/copyleft/boolean/*.h ${TEST_ROOT_DIR}/include/igl/copyleft/cgal/*.h ${TEST_ROOT_DIR}/include/igl/copyleft/boolean/*.inl ${TEST_ROOT_DIR}/include/igl/copyleft/cgal/*.inl)
+  target_sources(libigl_tests PRIVATE ${TEST_SRC_FILES} ${TEST_INC_FILES})
+
+  target_link_libraries(libigl_tests PUBLIC igl::igl::cgal)


Should be target_link_libraries(libigl_tests PUBLIC igl::cgal)

jdumas · 2018-10-19T22:27:34Z

tests/include/igl/avg_edge_length.cpp

@@ -31,32 +31,32 @@ TEST(avg_edge_length, cube)
  double avg;

  avg = igl::avg_edge_length(V,F);
-  ASSERT_NEAR((12.*sqrt(side_sq) + 6.*sqrt(diag_sq))/(12.+6.), avg, epsilon);
+  REQUIRE (avg == Approx ((12.*sqrt(side_sq) + 6.*sqrt(diag_sq))/(12.+6.)).margin( epsilon));


I would remove the space between the macro and the parenthesis (to keep it consistent).

this part was autogenerated with a script. There might be a lot of cases like that. I dont think it is worth to time investment

find tests/ -exec sed -i 's/REQUIRE (/REQUIRE(/g' {} \; (may need to escape the ().

On mac you need sed -i "" 's/...' instead.

teseoch · 2018-10-22T20:07:29Z

I fixed the couple of stuff in the cmake and reneabled the test with parameters.

With respect to the old version, it generates only 1 test case per set of parameters. I think it makes more sense.

danielepanozzo · 2018-10-23T18:56:36Z

Mmm it seems to me that all tests have been ported (except bbw on windows which has some issues, but the test passes on mac and linux). @teseoch could you confirm? If that's the case I think we should merge this ASAP and polish it later. This PR is blocking all the other ones that require unit tests.

alecjacobson · 2018-10-23T19:00:42Z

We also need a well documented conversion script for PRs or other existing gtests.

…

On Tue, Oct 23, 2018 at 20:56 Daniele Panozzo ***@***.***> wrote: Mmm it seems to me that all tests have been ported (except bbw on windows which has some issues, but the test passes on mac and linux). @teseoch <https://github.com/teseoch> could you confirm? If that's the case I think we should merge this ASAP and polish it later. This PR is blocking all the other ones that require unit tests. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#961 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACI0mRfY1xj3fe4cn4UocWL59sK0AN_Aks5un2ZygaJpZM4Xxg8m> .

jdumas · 2018-10-23T19:08:15Z

There's only one other PRs (for embree). But the documentation on the website for the unit tests is a bit outdated (it still refers to the old repo).

alecjacobson · 2018-10-23T19:17:14Z

I have many tests that I haven’t pushed yet. We should document the conversion process.

…

On Tue, Oct 23, 2018 at 21:08 Jérémie Dumas ***@***.***> wrote: There's only one other PRs (for embree). But the documentation on the website for the unit tests is a bit outdated (it still refers to the old repo). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#961 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACI0mQxuBJ20kGfO-kcs6z1Wd93DgcDgks5un2kmgaJpZM4Xxg8m> .

teseoch · 2018-10-23T19:28:38Z

I have used this script to convert gtest into catch2 testes
MigrateFromGoogleTestToCatch.py.zip

danielepanozzo · 2018-10-24T13:53:46Z

This looks good to me and the script is a good way to convert old tests. @alecjacobson can we merge? I would like to enable the testing on windows which depends on this PR.

jdumas · 2018-10-24T14:16:10Z

Enabling the testing on Windows is independent from this PR, it's just a matter of setting proper flags in the CMake.

fwilliams · 2018-10-24T15:41:32Z

Can we merge #952 (which uses Google Test) before we merge this PR?

teseoch · 2018-10-24T22:00:27Z

@alecjacobson can you review your review?

qnzhou · 2018-10-26T04:05:13Z

cmake/LibiglDownloadExternal.cmake

@@ -134,10 +134,10 @@ function(igl_download_triangle)
 endfunction()

 ## Google test


Out of date comment.

qnzhou · 2018-10-26T04:46:02Z

I also have some unit tests that I want to create and push, but maybe it is better to wait until this pr is merged?

jdumas · 2018-10-26T04:48:00Z

It'd make sense to merge this one first and then convert existing PRs and use Catch2 for writing new tests.

qnzhou · 2018-10-26T04:51:20Z

Also, catch2 has the interesting tag feature. Currently, we are using the same tag, [igl], for most test cases. We could create some convention on tagging unit tests so it is easier to run a specific subset of tests.

jdumas · 2018-10-26T12:18:00Z

Agreed. Can have one tag per module for now (core, cgal, embree, etc.).

teseoch · 2018-10-26T21:47:06Z

i am using the suite name for the different modules eg TEST_CASE("CSGTree: extrusion", "[igl/copyleft/cgal]")

shall we merge?

alecjacobson · 2018-10-27T16:48:58Z

@teseoch does that script work for TEST and TEST_P ?

teseoch · 2018-10-29T14:22:06Z

@alecjacobson no the script dosent work with TEST_P.

To change the TEST_P you need to move the content of the TEST_P in a lambda like const auto test_case = [](const std::string &param), then call test_common::run_test_cases(test_common::all_meshes(), test_case); where the first parameter is the stuff inside INSTANTIATE_TEST_CASE_P.

You can look at the this file:
https://github.com/libigl/libigl/pull/961/files#diff-10f6cbc342547d20dcb837dc856f9912
to see the small changes

alecjacobson · 2018-10-29T14:23:27Z

Is it possible to script this? There are some new tests that need to be converted.

jdumas · 2018-10-29T14:32:02Z

Unless you have >10 new tests that use TEST_P, it might be faster to convert them by hand. The script converts the normal TEST though.

teseoch · 2018-10-29T20:41:23Z

can we merge before pushing new tests?

updated gtest to catch

aa40b90

alecjacobson requested changes Oct 19, 2018

View reviewed changes

jdumas reviewed Oct 19, 2018

View reviewed changes

jdumas mentioned this pull request Oct 22, 2018

Unexpected behavior of segments_intersect in segment_segment_intersect.h #957

Closed

3 tasks

cleand cmake and added INSTANTIATE_TEST_CASE_P

49a7459

fixed typo in cmake test

6b13d5a

danielepanozzo approved these changes Oct 24, 2018

View reviewed changes

jdumas mentioned this pull request Oct 24, 2018

Add extra exceptions for arithmetic and types operations in UnitTests #980

Open

1 task

jdumas added the unit-tests label Oct 25, 2018

jdumas requested a review from qnzhou October 25, 2018 22:42

qnzhou reviewed Oct 26, 2018

View reviewed changes

jdumas mentioned this pull request Oct 28, 2018

delaunay unit tests, expose is_delaunay, templates, PlainObject -> Matrix #956

Merged

5 tasks

teseoch added 2 commits October 29, 2018 15:59

Merge branch 'dev' into catch2

6f41773

ported new tests to catch2

1c3c7c7

alecjacobson approved these changes Oct 29, 2018

View reviewed changes

alecjacobson merged commit 7f7551a into libigl:dev Oct 29, 2018

teseoch deleted the catch2 branch October 29, 2018 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed from google test to catch #961

Changed from google test to catch #961

teseoch commented Oct 19, 2018 •

edited

alecjacobson left a comment

jdumas Oct 19, 2018 •

edited

jdumas Oct 19, 2018

jdumas Oct 19, 2018 •

edited

jdumas Oct 19, 2018

teseoch Oct 22, 2018

jdumas Oct 22, 2018

jdumas Oct 22, 2018

teseoch commented Oct 22, 2018

danielepanozzo commented Oct 23, 2018

alecjacobson commented Oct 23, 2018 via email

jdumas commented Oct 23, 2018

alecjacobson commented Oct 23, 2018 via email

teseoch commented Oct 23, 2018

danielepanozzo commented Oct 24, 2018

jdumas commented Oct 24, 2018

fwilliams commented Oct 24, 2018

teseoch commented Oct 24, 2018

qnzhou Oct 26, 2018

qnzhou commented Oct 26, 2018

jdumas commented Oct 26, 2018

qnzhou commented Oct 26, 2018

jdumas commented Oct 26, 2018

teseoch commented Oct 26, 2018 •

edited

alecjacobson commented Oct 27, 2018

teseoch commented Oct 29, 2018

alecjacobson commented Oct 29, 2018

jdumas commented Oct 29, 2018

teseoch commented Oct 29, 2018

		@@ -134,10 +134,10 @@ function(igl_download_triangle)
		endfunction()

		## Google test

Changed from google test to catch #961

Changed from google test to catch #961

Conversation

teseoch commented Oct 19, 2018 • edited

Check all that apply (change to [x])

alecjacobson left a comment

Choose a reason for hiding this comment

jdumas Oct 19, 2018 • edited

Choose a reason for hiding this comment

jdumas Oct 19, 2018

Choose a reason for hiding this comment

jdumas Oct 19, 2018 • edited

Choose a reason for hiding this comment

jdumas Oct 19, 2018

Choose a reason for hiding this comment

teseoch Oct 22, 2018

Choose a reason for hiding this comment

jdumas Oct 22, 2018

Choose a reason for hiding this comment

jdumas Oct 22, 2018

Choose a reason for hiding this comment

teseoch commented Oct 22, 2018

danielepanozzo commented Oct 23, 2018

alecjacobson commented Oct 23, 2018 via email

jdumas commented Oct 23, 2018

alecjacobson commented Oct 23, 2018 via email

teseoch commented Oct 23, 2018

danielepanozzo commented Oct 24, 2018

jdumas commented Oct 24, 2018

fwilliams commented Oct 24, 2018

teseoch commented Oct 24, 2018

qnzhou Oct 26, 2018

Choose a reason for hiding this comment

qnzhou commented Oct 26, 2018

jdumas commented Oct 26, 2018

qnzhou commented Oct 26, 2018

jdumas commented Oct 26, 2018

teseoch commented Oct 26, 2018 • edited

alecjacobson commented Oct 27, 2018

teseoch commented Oct 29, 2018

alecjacobson commented Oct 29, 2018

jdumas commented Oct 29, 2018

teseoch commented Oct 29, 2018

teseoch commented Oct 19, 2018 •

edited

Check all that apply (change to `[x]`)

jdumas Oct 19, 2018 •

edited

jdumas Oct 19, 2018 •

edited

teseoch commented Oct 26, 2018 •

edited