Improve test coverage#29
Conversation
…ing, verbose flag
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #29 +/- ##
==========================================
Coverage ? 65.58%
==========================================
Files ? 23
Lines ? 1784
Branches ? 740
==========================================
Hits ? 1170
Misses ? 522
Partials ? 92
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,42 @@ | |||
| #ifndef RAMCORE_RAMSTATS_H | |||
| #define RAMCORE_RAMSTATS_H | |||
There was a problem hiding this comment.
warning: header guard does not follow preferred style [llvm-header-guard]
| #define RAMCORE_RAMSTATS_H | |
| #ifndef GITHUB_WORKSPACE_INC_RAMCORE_RAMSTATS_H | |
| #define GITHUB_WORKSPACE_INC_RAMCORE_RAMSTATS_H |
inc/ramcore/RAMStats.h:41:
+ endif // GITHUB_WORKSPACE_INC_RAMCORE_RAMSTATS_H| #ifndef RAMCORE_RAMSTATS_H | ||
| #define RAMCORE_RAMSTATS_H | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
warning: 'cstdint' file not found [clang-diagnostic-error]
#include <cstdint>
^|
|
||
| #include <cstdint> | ||
| #include <string> | ||
| #include <map> |
There was a problem hiding this comment.
warning: #includes are not sorted properly [llvm-include-order]
| #include <map> | |
| #include <map> | |
| #include <string> |
| #include <string> | ||
| #include <map> | ||
|
|
||
| namespace ramcore { |
There was a problem hiding this comment.
warning: variable 'ramcore' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
namespace ramcore {
^| @@ -0,0 +1,145 @@ | |||
| #include "ramcore/RAMStats.h" | |||
There was a problem hiding this comment.
warning: 'ramcore/RAMStats.h' file not found [clang-diagnostic-error]
#include "ramcore/RAMStats.h"
^| #include <cstring> | ||
| #include <exception> | ||
| #include <iomanip> | ||
| #include <iostream> |
There was a problem hiding this comment.
warning: included header iomanip is not used directly [misc-include-cleaner]
| #include <iostream> | |
| #include <iostream> |
| #include <exception> | ||
| #include <iomanip> | ||
| #include <iostream> | ||
| #include <map> |
There was a problem hiding this comment.
warning: included header iostream is not used directly [misc-include-cleaner]
| #include <map> | |
| #include <map> |
| #include <iomanip> | ||
| #include <iostream> | ||
| #include <map> | ||
| #include <memory> |
There was a problem hiding this comment.
warning: included header map is not used directly [misc-include-cleaner]
| #include <memory> | |
| #include <memory> |
| #include <iostream> | ||
| #include <map> | ||
| #include <memory> | ||
| #include <string> |
There was a problem hiding this comment.
warning: included header memory is not used directly [misc-include-cleaner]
| #include <string> | |
| #include <string> |
| return length; | ||
| } | ||
|
|
||
| RAMStatsResult ComputeStats(const char *filename) |
There was a problem hiding this comment.
warning: function 'ComputeStats' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| RAMStatsResult ComputeStats(const char *filename) | |
| RAMStatsResult ComputeStatsstatic (const char *filename) |
| @@ -0,0 +1,11 @@ | |||
| #ifndef RAMCORE_RAMSORT_H | |||
| #define RAMCORE_RAMSORT_H | |||
There was a problem hiding this comment.
warning: header guard does not follow preferred style [llvm-header-guard]
| #define RAMCORE_RAMSORT_H | |
| #ifndef GITHUB_WORKSPACE_INC_RAMCORE_RAMSORT_H | |
| #define GITHUB_WORKSPACE_INC_RAMCORE_RAMSORT_H |
inc/ramcore/RAMSort.h:10:
- #endif // RAMCORE_RAMSORT_H
+ #endif // GITHUB_WORKSPACE_INC_RAMCORE_RAMSORT_H| /// \param outputFile Path to output .root RAM file | ||
| /// \param byName If true, sort by QNAME; otherwise sort by (refid, pos) | ||
| /// \return 0 on success, 1 on error | ||
| int ramsortntuple(const char *inputFile, const char *outputFile, bool byName = false); |
There was a problem hiding this comment.
warning: unknown type name 'bool' [clang-diagnostic-error]
int ramsortntuple(const char *inputFile, const char *outputFile, bool byName = false);
^| /// \param outputFile Path to output .root RAM file | ||
| /// \param byName If true, sort by QNAME; otherwise sort by (refid, pos) | ||
| /// \return 0 on success, 1 on error | ||
| int ramsortntuple(const char *inputFile, const char *outputFile, bool byName = false); |
There was a problem hiding this comment.
warning: use of undeclared identifier 'false' [clang-diagnostic-error]
int ramsortntuple(const char *inputFile, const char *outputFile, bool byName = false);
^| @@ -0,0 +1,92 @@ | |||
| #include "ramcore/RAMSort.h" | |||
There was a problem hiding this comment.
warning: 'ramcore/RAMSort.h' file not found [clang-diagnostic-error]
#include "ramcore/RAMSort.h"
^|
|
||
| #include <ROOT/RNTupleModel.hxx> | ||
| #include <ROOT/RNTupleReader.hxx> | ||
| #include <ROOT/RNTupleWriter.hxx> |
There was a problem hiding this comment.
warning: #includes are not sorted properly [llvm-include-order]
| #include <ROOT/RNTupleWriter.hxx> | |
| #include <ROOT/RNTupleView.hxx> |
src/ramcore/RAMSort.cxx:7:
- #include <ROOT/RNTupleView.hxx>
+ #include <ROOT/RNTupleWriter.hxx>| #include <iostream> | ||
| #include <numeric> | ||
| #include <string> | ||
| #include <vector> |
There was a problem hiding this comment.
warning: included header string is not used directly [misc-include-cleaner]
| #include <vector> | |
| #include <vector> |
| #include <numeric> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
There was a problem hiding this comment.
warning: included header vector is not used directly [misc-include-cleaner]
| #include <string> | ||
| #include <vector> | ||
|
|
||
| int ramsortntuple(const char *inputFile, const char *outputFile, bool byName) |
There was a problem hiding this comment.
warning: function 'ramsortntuple' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| int ramsortntuple(const char *inputFile, const char *outputFile, bool byName) | |
| static int ramsortntuple(const char *inputFile, const char *outputFile, bool byName) |
| std::unique_ptr<ROOT::RNTupleReader> reader; | ||
| try { | ||
| reader = ROOT::RNTupleReader::Open("RAM", inputFile); | ||
| } catch (const std::exception &e) { |
There was a problem hiding this comment.
warning: no header providing "std::exception" is directly included [misc-include-cleaner]
src/ramcore/RAMSort.cxx:11:
- #include <iostream>
+ #include <exception>
+ #include <iostream>| std::unique_ptr<ROOT::RNTupleReader> reader; | ||
| try { | ||
| reader = ROOT::RNTupleReader::Open("RAM", filename); | ||
| } catch (const std::exception &e) { |
There was a problem hiding this comment.
warning: empty catch statements hide issues; to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
} catch (const std::exception &e) {
^
|
Hi @swetank18! Let me know if my understanding is incorrect. |
| @@ -0,0 +1,48 @@ | |||
| #ifndef RAMCORE_RAMSTATS_H | |||
| #define RAMCORE_RAMSTATS_H | |||
There was a problem hiding this comment.
warning: header guard does not follow preferred style [llvm-header-guard]
| #define RAMCORE_RAMSTATS_H | |
| #ifndef GITHUB_WORKSPACE_INC_RAMCORE_RAMSTATS_H | |
| #define GITHUB_WORKSPACE_INC_RAMCORE_RAMSTATS_H |
inc/ramcore/RAMStats.h:47:
+ endif // GITHUB_WORKSPACE_INC_RAMCORE_RAMSTATS_H|
|
||
| #include <ROOT/RNTupleModel.hxx> | ||
| #include <ROOT/RNTupleReader.hxx> | ||
| #include <ROOT/RNTupleWriter.hxx> |
There was a problem hiding this comment.
warning: #includes are not sorted properly [llvm-include-order]
#include <ROOT/RNTupleWriter.hxx>
^this fix will not be applied because it overlaps with another fix
|
|
||
| #include <ROOT/RNTupleModel.hxx> | ||
| #include <ROOT/RNTupleReader.hxx> | ||
| #include <ROOT/RNTupleWriter.hxx> |
There was a problem hiding this comment.
warning: included header RNTupleReader.hxx is not used directly [misc-include-cleaner]
| #include <ROOT/RNTupleWriter.hxx> | |
| #include <ROOT/RNTupleWriter.hxx> |
| #include <ROOT/RNTupleWriter.hxx> | ||
| #include <ROOT/RNTupleWriteOptions.hxx> | ||
| #include <ROOT/RNTupleView.hxx> | ||
| #include <TFile.h> |
There was a problem hiding this comment.
warning: included header RNTupleView.hxx is not used directly [misc-include-cleaner]
| #include <TFile.h> | |
| #include <TFile.h> |
| { | ||
| RAMNTupleRecord::ReadAllRefs(inputFile); | ||
|
|
||
| std::unique_ptr<ROOT::RNTupleReader> reader; |
There was a problem hiding this comment.
warning: no header providing "std::unique_ptr" is directly included [misc-include-cleaner]
src/ramcore/RAMSort.cxx:12:
- #include <numeric>
+ #include <memory>
+ #include <numeric>|
|
||
| std::unique_ptr<ROOT::RNTupleReader> reader; | ||
| try { | ||
| reader = ROOT::RNTupleReader::Open("RAM", inputFile); |
There was a problem hiding this comment.
warning: incomplete type 'ROOT::RNTupleReader' named in nested name specifier [clang-diagnostic-error]
reader = ROOT::RNTupleReader::Open("RAM", inputFile);
^Additional context
inc/rntuple/RAMNTupleRecord.h:23: forward declaration of 'ROOT::RNTupleReader'
class RNTupleReader;
^| return 1; | ||
| } | ||
|
|
||
| const uint64_t nEntries = reader->GetNEntries(); |
There was a problem hiding this comment.
warning: member access into incomplete type 'ROOT::RNTupleReader' [clang-diagnostic-error]
const uint64_t nEntries = reader->GetNEntries();
^Additional context
inc/rntuple/RAMNTupleRecord.h:23: forward declaration of 'ROOT::RNTupleReader'
class RNTupleReader;
^| return 1; | ||
| } | ||
|
|
||
| const uint64_t nEntries = reader->GetNEntries(); |
There was a problem hiding this comment.
warning: no header providing "uint64_t" is directly included [misc-include-cleaner]
src/ramcore/RAMSort.cxx:11:
- #include <iostream>
+ #include <cstdint>
+ #include <iostream>| return 1; | ||
| } | ||
|
|
||
| auto viewRefId = reader->GetView<int32_t>("record.refid"); |
There was a problem hiding this comment.
warning: member access into incomplete type 'ROOT::RNTupleReader' [clang-diagnostic-error]
auto viewRefId = reader->GetView<int32_t>("record.refid");
^Additional context
inc/rntuple/RAMNTupleRecord.h:23: forward declaration of 'ROOT::RNTupleReader'
class RNTupleReader;
^| return 1; | ||
| } | ||
|
|
||
| auto viewRefId = reader->GetView<int32_t>("record.refid"); |
There was a problem hiding this comment.
warning: no header providing "int32_t" is directly included [misc-include-cleaner]
auto viewRefId = reader->GetView<int32_t>("record.refid");
^
Hi, thank you for reading through the PR carefully! New functionality ramstats tool and ramsort tool with their tests (these are better suited to their own PRs, which I have open as #28 and #30) You're right that the PR description was misleading by mixing both concerns together. The part that genuinely addresses issue #26 is samparsertest.cxx it adds tests for pre-existing code that had no coverage. |
I recommend splitting it; otherwise, the PR title is a bit misleading. |
3 participants
Closes #26
Summary
This PR improves test coverage for the
ramcorelibrary by adding a comprehensive test suite for theRAMStatsfunctionality andSamParserintegration.Changes
New files:
inc/ramcore/RAMStats.h—RAMStatsstruct,RAMStatsResultwrapper, andComputeStats()declarationsrc/ramcore/RAMStats.cxx— implementation usingRNTupleReaderfield views with safe error handlingtools/ramstats.cxx— command-line tool equivalent tosamtools flagstattest/ramstatstests.cxx— 9 Google Test cases covering RAMStatsModified files:
CMakeLists.txt— addedRAMStats.handRAMStats.cxxto theramcorelibrarytools/CMakeLists.txt— addedramstatsexecutabletest/CMakeLists.txt— addedramstatstestsandsamparsertesttest targetsTests Added
9 test cases in
test/ramstatstests.cxx— all passing:TotalReadsMatchesNTupleEntries— cross-validates againstRNTupleReader::GetNEntries()MappedPlusUnmappedEqualsTotal— ensures no reads are lostStrandCountsEqualTotal— forward + reverse = totalMeanReadLengthIsPositive— sanity check on non-empty fileTotalBasesConsistentWithMeanLength— floating-point consistencyMeanMappingQualityInValidRange— SAM spec [0, 255]ReadsPerChromosomeNotEmpty— chromosome map populatedChromosomeCountsSumToAtMostTotal— unmapped reads excluded correctlyNonExistentFileReturnsEmpty— graceful failure on bad inputTest Results
Codecov
Patch coverage: 88.57% — above the 85% threshold required by
.codecov.yml.Implementation Notes
RNTupleReaderfield views (record.flag,record.mapq,record.seq,record.refid) for efficient columnar accessRAMNTupleRecord::ReadAllRefs()andRAMNTupleRefsRNTupleReader::Openwrapped in try/catch for safe error handlingramcorelibrary