Add more fortran wrapper tests#232
Add more fortran wrapper tests#232atztogo merged 8 commits intospglib:developfrom atztogo:fortran-test
Conversation
Codecov ReportBase: 93.34% // Head: 93.34% // No change to project coverage 👍
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## develop #232 +/- ##
========================================
Coverage 93.34% 93.34%
========================================
Files 15 15
Lines 902 902
========================================
Hits 842 842
Misses 60 60 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
LecrisUT
left a comment
There was a problem hiding this comment.
What limitations were you trying to overcome here or how do you want to manage the tests?
| # Add fortran tests here | ||
| create_test_sourcelist(FortranTests_Files | ||
| ${TestsDriver} | ||
| test_fortran_spg_get_symmetry.f90) |
There was a problem hiding this comment.
I would prefer to either use create_test_sourcelist or use an upstream Fortran unit tester like Fortuno.
This would minimize the executables displayed in IDEs, faster to build, and it is easier to add tests (less poilerplate compared to writing Program for each one)
There was a problem hiding this comment.
I see your point.
I could not figure out how to put multiple test functions in one file, which is for easier writing and reading the test code. Using create_test_sourcelist, it registers one function per one file, as far as I tried. So if I want to add more tests, I thought I had to create a number of files. If this point is overcome, surely what you explained is better.
There was a problem hiding this comment.
Totally agree with you. I would like it in a similar file as well.
Here is a design choice to consider though. Do we want ctest to be able to call subfunctions as well?
For example:
# Loop over all main tests
foreach (test ${TestsToRun})
get_filename_component(test_name ${test} NAME_WE)
# Check if we have subtests defined
if (${SubTests_${test_name}})
# Loop over all subtests
foreach (subtest ${SubTests_${test_name}}
# Call test via ${FortranTests_Executable} ${test_name} ${subtest}
# E.g.: ./FortranTests_Executable test_fortran_spg_get_symmetry test_rutile112
add_test(NAME "${test_name}/${subtest}"
COMMAND $<TARGET_FILE:FortranTests> ${test_name} ${subtest})
endforeach ()
else ()
# Only run main test
add_test(NAME "${test_name}"
COMMAND $<TARGET_FILE:FortranTests> ${test_name})
endif ()
endforeach ()I can show you how to implement that from the argc/argv if you need help on that.
There was a problem hiding this comment.
I see. It is a good trick. If I understand it correctly, I need two lists, ${TestsToRun} and ${SubTests_${test_name}. Am I correct?
I can show you how to implement that from the argc/argv if you need help on that.
Yes, I need your help. I would appreciate it.
There was a problem hiding this comment.
Ok, I'll push a commit directly here with an example for test_fortran_spg_get_symmetry maybe tonight or tomorrow.
|
|
||
| num_atom_bravais = spg_refine_cell(lattice, position, types, num_atom, symprec) | ||
|
|
||
| call assert_int(num_atom_bravais, 4) |
There was a problem hiding this comment.
Maybe worth making a:
interface assert
module procedure assert_int, assert_...
end interfaceThere was a problem hiding this comment.
I agree! I am not yet familiar with fortran and just learning right now. Hopefully this will be done at near future.
There was a problem hiding this comment.
Yeah Fortran is a mess of a language, I can barely get a footing into it without cursing
| if (spglib_WITH_TESTS) | ||
| if(spglib_WITH_Fortran) | ||
| set(CMAKE_Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/fortran) | ||
| endif() | ||
| add_subdirectory(test) | ||
| endif () |
There was a problem hiding this comment.
Yeah, just add an if (NOT) statement so that devs can choose their own workflow, like this:
| if (spglib_WITH_TESTS) | |
| if(spglib_WITH_Fortran) | |
| set(CMAKE_Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/fortran) | |
| endif() | |
| add_subdirectory(test) | |
| endif () | |
| if (NOT CMAKE_Fortran_MODULE_DIRECTORY) | |
| set(CMAKE_Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/fortran_mods) | |
| endif () |
Naming is arbitrary, that's what I implement in octopus.
As for the other fix in spglib_f08, I was refering to these lines:
The target_include_directories there is so that the Fortran compilers includes the folder where the .mod file is, either CMAKE_Fortran_MODULE_DIRECTORY during when we build locally (BUILD_INTERFACE), or the location where we installed it to (CMAKE_INSTALL_INCLUDEDIR at INSTALL_INTERFACE). So something like this:
target_include_directories(spglib_f08 PUBLIC
"$<BUILD_INTERFACE:${CMAKE_Fortran_MODULE_DIRECTORY}>"
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>")
One thing I didn't test, but it seemed to be working is that for Fortran modules (maybe even C++ modules in the future), we do not need to include the folder with the .F90 file (as we do with .h in C/C++). In principle we could remove spglib_f08.F90 from the install, but we need to test it out a bit further.
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
|
Ok, a bit slow, but here is the implementation. I've added some labels to select the tests like |
Intel always links to its own MAIN__ definition. This disables it for `FortranTests` because MAIN__ is defined in `fortran_test.c` Signed-off-by: Cristian Le <[email protected]>
|
Fixes #234 |
|
Sorry I was a little bit busy not to work on this. @LecrisUT, to include your commits (I'm interested in how you add commits in this PR), can we merge this PR at this stage? This is because I am not good at handling github PR, and don't know how I can continue this PR to add more fix following your advice given two weeks ago. So merging, then making a new PR looks a reasonable way for me. |
|
When yoy make a Github PR, you can set "Maintainers are allowed to edit this pull request.” that allows project collaborators to add commits to their (the person submitting the PR) fork. So you can continue to work on your branch, simply pull and merge/rebase your changes on top of it. But, however it is more easier for you to work on. We can merge this and add a new branch/PR to continue to work on it. |
|
Merged. Thanks @LecrisUT. |
Motivation is to make each
test_fortran_SPG_FUNCTION_NAME.f90contain multiple test functions (or subroutines) that use the same corresponding spglib function (or subroutine). An example istest_fortran_spg_get_symmetry.f90. Different set of test for different spglib function is written in another file (e.g.,test_fortran_spg_refine_cell.f90).This is not an elegant approach, but it is OK enough for me. If this way is reasonable, I will add more tests.