[CCPPETMR/SIRF] added tests (#58)
Edoardo Pasca
Add test phase to the SIRF project.
You can view, comment on, or merge this pull request online at:
https://github.com/CCPPETMR/SIRF/pull/58
Commit Summary
- added tests
File Changes
- M CMakeLists.txt (10)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
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).
Edoardo Pasca
@paskino pushed 1 commit.
- f7b8a4a Moved test definition to subdirectory
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Kris Thielemans
@KrisThielemans requested changes on this pull request.
some clean-up needed
In src/xSTIR/pSTIR/tests/CMakeLists.txt:
> +# +# This file is part of the CCP PETMR Synergistic Image Reconstruction Framework (SIRF) SuperBuild. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0.txt +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +#=========================================================================
forgot to add the actual test?
In 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
In src/xGadgetron/pGadgetron/tests/CMakeLists.txt:
> +# This file is part of the CCP PETMR Synergistic Image Reconstruction Framework (SIRF) SuperBuild. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0.txt +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +#========================================================================= +ENABLE_TESTING()
no need for this as in top-level (probably should be there only)
In CMakeLists.txt:
> @@ -74,15 +74,15 @@ endif()
#### enable support for ctest
ENABLE_TESTING()
-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...
Edoardo Pasca
@paskino pushed 1 commit.
- 583ad8a cleanup
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
@paskino pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
I have moved the tests in src/xGadgetron/pGadgetron/tests/CMakeLists.txt and src/xSTIR/pSTIR/tests/CMakeLists.txt
For some reasons I did not figure out yet, the STIR tests are not found.
Kris Thielemans
@KrisThielemans commented on this pull request.
In CMakeLists.txt:
> @@ -74,6 +74,16 @@ endif() #### enable support for ctest ENABLE_TESTING() +#include(FindPythonInterp)
let's remove these comments
Kris Thielemans
looks good but a few comments remaining.
no clue why it doesn't find the pStir test. sorry.
Edoardo Pasca
@paskino pushed 1 commit.
- 92b3f55 removed comments
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
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.
Kris Thielemans
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.
Edoardo Pasca
@paskino pushed 1 commit.
- 5ea74eb enable_testing moved to main CMakeLists.txt
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Kris Thielemans
perfect. ready to accept? Prefer a merge or a squash-merge? (see email)
Edoardo Pasca
I guess squash-merge
Edoardo Pasca
@paskino pushed 1 commit.
- 5b011e2 add enable_testing in the testing directory
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Kris Thielemans
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?
Kris Thielemans
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.
Edoardo Pasca
@paskino pushed 1 commit.
- eec9ce2 added path to tests
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
@paskino pushed 1 commit.
- 9450e55 cleanup test
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Edoardo Pasca
good to know! I actually struggled to find that variable! All right, I'll fix it next week then.
Kris Thielemans
Edoardo Pasca
@paskino pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Kris Thielemans
@KrisThielemans approved this pull request.
Kris Thielemans
Merged #58.
Casper da Costa-Luis
@casperdcl commented on this pull request.
In src/xGadgetron/pGadgetron/tests/CMakeLists.txt:
> +# This file is part of the CCP PETMR Synergistic Image Reconstruction Framework (SIRF) SuperBuild. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0.txt
+# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +#========================================================================= +include(FindPythonInterp)
@KrisThielemans I'm assuming this is overriding travis' PYTHON_EXECUTABLE
Casper da Costa-Luis
@casperdcl commented on this pull request.
In src/xSTIR/pSTIR/tests/CMakeLists.txt:
> +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0.txt
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+#=========================================================================
+include(FindPythonInterp)
+add_test(NAME PET_TEST1 COMMAND ${PYTHON_EXECUTABLE} test1.py WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/src/xSTIR/pSTIR/tests/ )
same here...
Edoardo Pasca
@paskino commented on this pull request.
In src/xGadgetron/pGadgetron/tests/CMakeLists.txt:
> +# This file is part of the CCP PETMR Synergistic Image Reconstruction Framework (SIRF) SuperBuild. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0.txt
+# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +#========================================================================= +include(FindPythonInterp)
why would this happen only on one of the systems in the travis matrix?