added tests #58

paskino · 2017-09-19T09:00:37Z

Add test phase to the SIRF project.

KrisThielemans

thanks. Move the tests to src/xGadgetron/pGadgetron/tests/CMakeLists.txt (and same for STIR). That's where we know what tests are available. (add an add_directory in the level above).

Now one adds tests definition in the appropriate x<>/p<>/tests/CMakeLists.txt

KrisThielemans

some clean-up needed

KrisThielemans · 2017-09-20T15:18:28Z

src/xSTIR/pSTIR/CMakeLists.txt

@@ -40,3 +40,5 @@ if(BUILD_PYTHON)
  INSTALL(FILES "${CMAKE_CURRENT_BINARY_DIR}/pystir.py" pStir.py DESTINATION "${PYTHON_DEST}")

 endif(BUILD_PYTHON)
+
+ADD_SUBDIRECTORY(tests)


move inside the if, to avoid this to be included if Python-stuff not built

KrisThielemans · 2017-09-20T15:18:58Z

src/xGadgetron/pGadgetron/tests/CMakeLists.txt

+# limitations under the License.
+#
+#=========================================================================
+ENABLE_TESTING()


no need for this as in top-level (probably should be there only)

KrisThielemans · 2017-09-20T15:19:31Z

CMakeLists.txt

-include(FindPythonInterp)
-add_test(NAME MR_FULLY_SAMPLED
-         COMMAND ${PYTHON_EXECUTABLE} src/xGadgetron/pGadgetron/tests/fully_sampled.py)
+#include(FindPythonInterp)


I prefer not to have the commented-out stuff here...

paskino · 2017-09-20T15:59:11Z

I have moved the tests in src/xGadgetron/pGadgetron/tests/CMakeLists.txt and src/xSTIR/pSTIR/tests/CMakeLists.txt

Unfortunately, for some reasons I did not figure out yet, the STIR tests are not found.

KrisThielemans · 2017-09-20T16:42:14Z

CMakeLists.txt

@@ -74,6 +74,16 @@ endif()

 #### enable support for ctest
 ENABLE_TESTING()
+#include(FindPythonInterp)


let's remove these comments

KrisThielemans · 2017-09-20T16:43:12Z

looks good but a few comments remaining.

no clue why it doesn't find the pStir test. sorry.

paskino · 2017-09-21T10:05:49Z

I checked today and it's fine. The problem I was facing yesterday was due to the fact that cmake didn't find STIR, hence the pSTIR tests were not run. It's all fine when built from the SuperBuild which sets all variables.
Once this is merged, we may merge the ctest branch on SuperBuild.

KrisThielemans · 2017-09-21T10:26:03Z

great. thanks.

Would you be able to put the enable_testing() in the main file, you've just commented it out :-; and remove it from the others? That looks a bit cleaner to me.

KrisThielemans · 2017-09-21T10:38:56Z

perfect. ready to accept? Prefer a merge or a squash-merge? (see email)

paskino · 2017-09-21T11:23:36Z

I guess squash-merge

without it the added tests are not added. This commit somehow undoes last commit.

currently the tests are not found in the directory of the build. they aren't there and this is a workaround to point the system to the right location: The system issues python from /home/sirfuser/devel/SIRF-SuperBuild/SIRF-prefix/src/SIRF-build/src/xGadgetron/pGadgetron/tests and there are no test files there. 3 Testing: MR_UNDER_SAMPLED 3/3 Test: MR_UNDER_SAMPLED Command: "/usr/bin/python2.7" "/home/sirfuser/devel/SIRF-SuperBuild/SIRF-prefix/src/ SIRF-build/src/xGadgetron/pGadgetron/tests/../../../../../../../SIRF/src/xGadgetron/ pGadgetron/tests/undersampled.py" Directory: /home/sirfuser/devel/SIRF-SuperBuild/SIRF-prefix/src/SIRF-build/src/xGadg etron/pGadgetron/tests

KrisThielemans · 2017-09-21T15:33:05Z

but the same doc says

Enables testing for this directory and below.

it doesn't make sense to me (and it isn't needed in STIR). @paskino, are you sure we need this?

KrisThielemans · 2017-09-21T15:35:23Z

By the way, you will need to merge master onto here for final testing. @evgueni-ovtchinnikov fixed filenames and "case" of directory, so this might affect this PR.

use WORKING_DIRECTORY directive and ${CMAKE_PREFIX_PATH} to determine the right location of the test files. It's not different as before but much better looking. Removed ADD_TESTING(). Discard my previous comment about this not working if in the main CMakeLists.txt

paskino · 2017-09-21T16:10:19Z

good to know! I actually struggled to find that variable! All right, I'll fix it next week then.

KrisThielemans · 2017-09-21T16:12:06Z

https://cmake.org/Wiki/CMake_Useful_Variables

use CMAKE_SOURCE_DIR rather than CMAKE_PREFIX_PATH to set the path to the test files.

casperdcl · 2017-09-28T12:20:58Z

src/xGadgetron/pGadgetron/tests/CMakeLists.txt

+# limitations under the License.
+#
+#=========================================================================
+include(FindPythonInterp)


@KrisThielemans I'm assuming this is overriding travis' PYTHON_EXECUTABLE

why would this happen only on one of the systems in the travis matrix?

casperdcl · 2017-09-28T12:21:17Z

src/xSTIR/pSTIR/tests/CMakeLists.txt

+#
+#=========================================================================
+include(FindPythonInterp)
+add_test(NAME PET_TEST1 COMMAND ${PYTHON_EXECUTABLE} test1.py WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/src/xSTIR/pSTIR/tests/ )


same here...

Add resampler docs

added tests

aa5e4b3

paskino requested review from KrisThielemans and evgueni-ovtchinnikov September 19, 2017 09:00

KrisThielemans requested changes Sep 19, 2017

View reviewed changes

Moved test definition to subdirectory

f7b8a4a

Now one adds tests definition in the appropriate x<>/p<>/tests/CMakeLists.txt

KrisThielemans requested changes Sep 20, 2017

View reviewed changes

paskino added 3 commits September 20, 2017 15:39

cleanup

583ad8a

add enable testing

e6c6195

fixed location of test file

9f93073

KrisThielemans reviewed Sep 20, 2017

View reviewed changes

removed comments

92b3f55

enable_testing moved to main CMakeLists.txt

5ea74eb

paskino added 2 commits September 21, 2017 11:40

add enable_testing in the testing directory

5b011e2

without it the added tests are not added. This commit somehow undoes last commit.

cleanup test

9450e55

use WORKING_DIRECTORY directive and ${CMAKE_PREFIX_PATH} to determine the right location of the test files. It's not different as before but much better looking. Removed ADD_TESTING(). Discard my previous comment about this not working if in the main CMakeLists.txt

paskino added 2 commits September 22, 2017 07:29

use CMAKE_SOURCE_DIR

4a1fc69

use CMAKE_SOURCE_DIR rather than CMAKE_PREFIX_PATH to set the path to the test files.

Merge branch 'master' into ctest

03adfb5

KrisThielemans approved these changes Sep 27, 2017

View reviewed changes

KrisThielemans merged commit 42e305f into master Sep 27, 2017

casperdcl reviewed Sep 28, 2017

View reviewed changes

KrisThielemans deleted the ctest branch October 13, 2017 23:02

johannesmayer pushed a commit to johannesmayer/SIRF that referenced this pull request Feb 21, 2022

Merge pull request SyneRBI#58 from AnderBiguri/resampler_docs

478b328

Add resampler docs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added tests #58

added tests #58

paskino commented Sep 19, 2017

KrisThielemans left a comment

KrisThielemans left a comment

KrisThielemans Sep 20, 2017

KrisThielemans Sep 20, 2017

KrisThielemans Sep 20, 2017

paskino commented Sep 20, 2017 •

edited

KrisThielemans Sep 20, 2017

KrisThielemans commented Sep 20, 2017

paskino commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

paskino commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

paskino commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

casperdcl Sep 28, 2017

paskino Sep 28, 2017

casperdcl Sep 28, 2017

added tests #58

added tests #58

Conversation

paskino commented Sep 19, 2017

KrisThielemans left a comment

Choose a reason for hiding this comment

KrisThielemans left a comment

Choose a reason for hiding this comment

KrisThielemans Sep 20, 2017

Choose a reason for hiding this comment

KrisThielemans Sep 20, 2017

Choose a reason for hiding this comment

KrisThielemans Sep 20, 2017

Choose a reason for hiding this comment

paskino commented Sep 20, 2017 • edited

KrisThielemans Sep 20, 2017

Choose a reason for hiding this comment

KrisThielemans commented Sep 20, 2017

paskino commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

paskino commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

paskino commented Sep 21, 2017

KrisThielemans commented Sep 21, 2017

casperdcl Sep 28, 2017

Choose a reason for hiding this comment

paskino Sep 28, 2017

Choose a reason for hiding this comment

casperdcl Sep 28, 2017

Choose a reason for hiding this comment

paskino commented Sep 20, 2017 •

edited