Update testing to use Catch2 and CMake #16

AidanBeltonS · 2022-08-31T13:07:03Z

This PR moves the testing away from using make and utility functions. It introduces Catch2 as a git submodule so the user does not have to separately build it. CMake is then used to build and run the tests. Using Catch2 reduces the amount of code needed to test SyclCPLX and will speed up adding new tests.

The readme has been updated to reflect this change.

TApplencourt · 2022-09-06T16:17:36Z

Back in vacation, will have a look :) Thanks!

TApplencourt · 2022-09-06T23:00:35Z

Should we use fetchContent? (https://github.com/catchorg/Catch2/blob/devel/docs/cmake-integration.md) I know nothing about CMAKE, but I know that I hate git submodule (always forgot to put the --recursive)

TApplencourt · 2022-09-06T23:03:51Z

tests/CMakeLists.txt

@@ -0,0 +1,45 @@
+file(GLOB test_cases CONFIGURE_DEPENDS "*.cpp")
+
+# set(test_cases 


I guess we can remove the comments now :)

Haha thanks for catching this. I have removed the comments

TApplencourt · 2022-09-06T23:05:52Z

tests/test_helper.hpp

+  return Q.get_device().has(sycl::aspect::fp64);
+}
+
+template <> inline bool is_type_supported<float>(sycl::queue &Q) {


I never realized that indeed fp32 is required by the SYCL spec :D

TApplencourt · 2022-09-06T23:10:01Z

tests/test_complex_types.cpp

-
-// Check functions return correct types
-void check_math_function_types() {
+TEST_MATH_FUNC_TYPE("Test complex acos types", "[acos]", acos)


Maybe a stupid question, but it seems strange to me that if we list all the TEST_MATH_FUNC_TYPE we need to add them manually in the CMAKELIST.
Sorry, I'm sure I'm missing something obvious...

So I don't quite understand. TEST_MATH_FUNC_TYPE is a macro that just allows the test to be repeated for all the different functions without me re-writing them. CMake needs a list of all the source files which then Catch2 parses for the tests Catch2 looks for a different macro something along the line of TEST_CASE("Test is_gencomplex", "[gencomplex]").
CMake needs a list of the test source files so it knows which files to compile otherwise it is will compile none of the tests files.
Does this answer your question, could you clarify if it does not?

Oh I see. Yes it clarifies; thanks! TEST_MATH_FUNC_TYPE(acos) is not calling or registering the tests inside acos_complex.cpp.

TEST_MATH_FUNC_TYPE are just another type of tests.

Thanks!

TApplencourt · 2022-09-06T23:14:40Z

tests/test_helper.hpp

+inline constexpr T inf_val = std::numeric_limits<T>::infinity();
+
+template <typename T>
+inline constexpr T nan_val = std::numeric_limits<T>::quiet_NaN();


Oh nice, I did knew about ::quiet_Nan(). I was using std::nan("") like a caveman :)

TApplencourt · 2022-09-06T23:19:40Z

tests/acosh_complex.cpp

+  sycl::ext::cplx::complex<T> cplx_input{input.re, input.im};

-    std::complex<T> std_out{init_re, init_im};
-    auto *cplx_out = sycl::malloc_shared<sycl::ext::cplx::complex<T>>(1, Q);
+  std::complex<T> std_out{input.re, input.im};


I guess we can just do std_out{input} (or maybe init_std_complex for taking care of half)

So unfortunately I cant do std_out{input} as input is of type cmplx which is just a helper struct of real and imaginary components. std::complex does not have a constructor for my helper struct cmplx.

Also in this case we don't want the half's to be be translated into floats as this is the output value which should have the half type. As no operation is being performed on the output type this is valid, the reason we are initializing it to the input values is for the case were we are checking if acosh can undo cosh. However when error codes are tested this value is overwritten.

Oh yes ofc! I'm stupid, it's a std::complex not our complex! Sorry, after coming back from vacation, my brain is slower than usual :)
Thanks for the explanation

TApplencourt · 2022-09-06T23:28:00Z

tests/acosh_complex.cpp

-    if (is_error_checking)
-      std_out = std::acosh(std_in);
+  // Get std::complex output
+  if (is_error_checking)


Not important but I think the is_type_supported should protect this guy too, as we can this lead to the variable is not used type of warning.

But thinking more can we use CTEST to only run those tests if is_type_supported is True? May avoid the duplication of is_type_supported call.

The tests assume that the type is supported, and it's ctests / catch2 responsibility to run then if possible. I don't know if it's possible, I know nothing about ctests / catch2...

I don't think is_type_supported should protect this, as lower down we test if the behaviour is valid on the host. While SYCL does say that a device does not need to support double and half types the host does so it makes sense not to calculate the reference value and have it for the later host check. It simply saves having two places where the reference value is calculated.

TApplencourt · 2022-09-06T23:31:14Z

Thanks for all ctest / catch integration. Look far better now :)

README.md

TApplencourt · 2022-09-07T15:13:16Z

tests/CMakeLists.txt

+file(GLOB test_cases CONFIGURE_DEPENDS "*.cpp")
+
+foreach(test_file IN LISTS test_cases)
+    # if(EXISTS "${test_file}")


Suggested change

# if(EXISTS "${test_file}")

I have uncommented these checks to make sure each file is available and emit a warning if it is not.

TApplencourt · 2022-09-07T15:13:30Z

tests/CMakeLists.txt

+    # else()
+        # message(FATAL_ERROR "No file named ${test_file}")
+    # endif()


Suggested change

# else()

# message(FATAL_ERROR "No file named ${test_file}")

# endif()

I have uncommented this check. Same as above.

TApplencourt · 2022-09-07T15:19:26Z

We saw 32% tests passed, 179 tests failed out of 262 with icpx / dpcpp. Look like oneAPI is not happy with CATCH2... Investigating right now. (all the old tests passed)

Thread 1 “abs_complex” received signal SIGSEGV, Segmentation fault.
0x0000000000407f65 in void CATCH2_INTERNAL_TEMPLATE_TEST_0<double>() ()
Missing separate debuginfos, use: zypper install exaperf_neo_agama-prerelease_579-22.36.024108.12156-main-debuginfo-1.0-1.x86_64 libdrm2-debuginfo-2.4.104-1.12.x86_64 libedit0-debuginfo-3.1.snap20150325-2.12.x86_64 liblzma5-debuginfo-5.2.3-4.3.1.x86_64 libncurses6-debuginfo-6.1-5.6.2.x86_64 libxml2-2-debuginfo-2.9.7-3.31.1.x86_64 libz1-debuginfo-1.2.11-3.21.1.x86_64
(gdb) where
#0 0x0000000000407f65 in void CATCH2_INTERNAL_TEMPLATE_TEST_0<double>() ()
#1 0x0000000000428e97 in Catch::RunContext::invokeActiveTestCase() ()
#2 0x0000000000426b34 in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) ()
#3 0x00000000004265ca in Catch::RunContext::runTest(Catch::TestCaseHandle const&) ()
#4 0x000000000042e16c in Catch::(anonymous namespace)::TestGroup::execute() ()
#5 0x000000000042c4d0 in Catch::Session::runInternal() ()
#6 0x000000000042bdd5 in Catch::Session::run() ()
#7 0x000000000040d991 in main ()

TApplencourt · 2022-09-07T17:16:52Z

Filled intel/llvm#6720

Co-authored-by: Thomas Applencourt <[email protected]>

AidanBeltonS · 2022-09-08T09:27:06Z

Thanks for filing the ticket. I do not have these issues locally. I am building dpcpp from the open source repo, so I guess the issue is specific to the distributed version of dpcpp.

TApplencourt · 2022-09-12T14:00:55Z

Thanks for filing the ticket. I do not have these issues locally. I am building dpcpp from the open source repo, so I guess the issue is specific to the distributed version of dpcpp.

Did you use the L0 backend? It's maybe backend specific. But We have a work-around ( just run the test one by one), so I will write a little script to do so, so I can continue testing with dpcpp.
EDIT: I Just tested dpcpp with OpenCL backend, and it still segfault. So I Agree with you, looks like a packaging issue.

Sorry for the late merge! And thanks again

AidanBeltonS · 2022-09-13T08:25:34Z

Thanks for filing the ticket. I do not have these issues locally. I am building dpcpp from the open source repo, so I guess the issue is specific to the distributed version of dpcpp.

Did you use the L0 backend? It's maybe backend specific. But We have a work-around ( just run the test one by one), so I will write a little script to do so, so I can continue testing with dpcpp. EDIT: I Just tested dpcpp with OpenCL backend, and it still segfault. So I Agree with you, looks like a packaging issue.

Sorry for the late merge! And thanks again

Yes, I have been testing with the L0 backend on a local machine

aidan.belton added 3 commits August 31, 2022 13:56

Add catch2

637bb3f

Update readme

d88d2e4

Set Catch2 to v2.x branch

75a5ffc

TApplencourt reviewed Sep 6, 2022

View reviewed changes

aidan.belton added 2 commits September 7, 2022 10:34

Undo Catch2 branch change

7829032

Update from feedback

32f652b

TApplencourt reviewed Sep 7, 2022

View reviewed changes

README.md Show resolved Hide resolved

TApplencourt reviewed Sep 7, 2022

View reviewed changes

AidanBeltonS and others added 2 commits September 8, 2022 09:40

Update README.md

5abb973

Co-authored-by: Thomas Applencourt <[email protected]>

Remove cmake comments

28c1d04

TApplencourt merged commit 33c7071 into argonne-lcf:main Sep 12, 2022

AidanBeltonS deleted the CMake branch September 13, 2022 08:25

		@@ -0,0 +1,45 @@
		file(GLOB test_cases CONFIGURE_DEPENDS "*.cpp")

		# set(test_cases

	# else()
	# message(FATAL_ERROR "No file named ${test_file}")
	# endif()

Update testing to use Catch2 and CMake #16

Update testing to use Catch2 and CMake #16

Uh oh!

Conversation

AidanBeltonS commented Aug 31, 2022

Uh oh!

TApplencourt commented Sep 6, 2022

Uh oh!

TApplencourt commented Sep 6, 2022

Uh oh!

TApplencourt Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt commented Sep 6, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TApplencourt commented Sep 7, 2022

Uh oh!

AidanBeltonS commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TApplencourt commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AidanBeltonS commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TApplencourt Sep 6, 2022 •

edited

Loading

TApplencourt Sep 6, 2022 •

edited

Loading

TApplencourt Sep 6, 2022 •

edited

Loading

TApplencourt Sep 7, 2022 •

edited

Loading

TApplencourt Sep 6, 2022 •

edited

Loading

TApplencourt commented Sep 7, 2022 •

edited

Loading

AidanBeltonS commented Sep 8, 2022 •

edited

Loading

TApplencourt commented Sep 12, 2022 •

edited

Loading