Code coverage and additional tests

[MurathanGoktas]

It was on our target list to implement code coverage testing to assure our code quality. #23

I made few changes to utilize the https://mesonbuild.com/Unit-tests.html automatic code coverage testing of Meson. However there are few things to have conversation on.

1. The automatic testing of meson also tests for code coverage of third party tools such as sdsl. That slows down the testing. We can alternatively run gcovr manually in scripts/code-coverage and speed up the process by manually targeting the files ourselves.
2. gcovr produces unreliable results for headers, thus currently at scripts/code-coverage I only assure .cpp files satisfy the code coverage threshold. However some of our classes do not have .cpp files but have only .hpp file such as mi-Bf. It is valuable to test them all. (Update: I seperated template classes to ".hpp" and ".tpp" and it is more reliable to test now)
3. To integrate this into CI we should fix GFA2 parser and its test, or remove its tests. Otherwise test fails. (Update: Removed GFA2 module)

[eli-schwartz]

The automatic testing of meson also tests for code coverage of third party tools such as sdsl. That slows down the testing. We can alternatively run gcovr manually in scripts/code-coverage and spped up the process by manually targeting the files ourselves.

Hmm, IIRC meson should already be passing -e subprojects to exclude gathering reports for your third-party dependencies.

[parham-k]

Since some templates have been moved to .tpp files, scripts/get-files should be modified to find */*.tpp. I'm guessing the .tpp files won't get copied to the installation dir here when ./compile is executed.

[MurathanGoktas]

Thanks @MurathanGoktas. The only thing I can think of is fixing the installation of .tpp files when ./compile is called.

I am not sure if its necessary to copy ".tpp" as it is included in ".hpp". Also when I changed scripts/get-files to include ".tpp" files it threw error, check https://github.com/mesonbuild/meson/issues/11697. What do you think @parham-k ?

[vlad0x00]

What's the advantage of using a different extension for templates? Generally, templates are simply put in .hpp files.

EDIT: Just saw the comment about code coverage. Are you saying that templates from a .hpp file are not included in coverage report? They should be covered if there's a corresponding test .cpp file that includes the .hpp.

[MurathanGoktas]

I was getting unreliable results for ".hpp" code coverage results. I wanted to separate the implementations to another file to get reliable code coverage results and also keep the code clean by having implementations seperately. According to this Stack Excange answer I seperated the code into ".hpp" and ".tpp" files. The ".tpp" files are specifically designated for templated classes, ensuring a clear and organized structure for our codebase.

[vlad0x00]

As meson discussion indicated, though, .tpp is not widely recognized. A better solution, as somebody said in the comments of your link, is to use two .hpp files, one with declarations and one with implementations. Even if you use .tpp extension, you still have to use the files as if they were .hpp, because template files need to be #includeed.

Although it doesn't strike me as unreadable to have declarations at the start of the .hpp file and implementations at its bottom.

[MurathanGoktas]

Alright, I have updated the file names accordingly, changing ".tpp" files to "-inl.hpp" files. Additionally, I verified that gcovr does not yield accurate results when we do not separate the implementations. It would be beneficial to develop a troubleshooting guide in the future to eliminate any ambiguities.

[MurathanGoktas]

Currently clang-tidy doesn't like that I have to .hpps for a class. As far as I can see there are 3 options:

1. use/NOLINT or some other thing suppress those.
2. go back to ".tpp" for implementations. Possible issue:

   Thanks @MurathanGoktas. The only thing I can think of is fixing the installation of .tpp files when ./compile is called.

3. combine ".hpp"s into one as it used to and have unreliable code coverage results for mi-BF and cBF classes. And exclude them from coverage assurance tests.

I am expecting a suggestion from reviewers otherwise I am tending to follow the second option.

An example of clang-tidy errors:

error: too many errors emitted, stopping now [clang-diagnostic-error]
/projects/btl/tgoktas/cc_btllib/btllib/include/btllib/counting_bloom_filter-inl.hpp:28:32: error: redefinition of 'CountingBloomFilter<T>' [clang-diagnostic-error]
inline CountingBloomFilter<T>::CountingBloomFilter(size_t bytes,
                               ^
/projects/btl/tgoktas/cc_btllib/btllib/include/btllib/counting_bloom_filter-inl.hpp:28:32: note: previous definition is here

[vlad0x00]

That error doesn't seem sensible. The redefinition is pointing to the same line. Does clang-tidy report the error exactly like that?

Also, redefinitions of functions should not happen. In fact, compiler should say so, not just clang-tidy. There's something fishy going on here. Spreading a class across multiple files should not be an issue.

[MurathanGoktas]

It was a fishy issue indeed, thanks @vlad0x00. This branch should be good to merge after resolving the conflicts.