Changes for test coverage#366
Conversation
📝 WalkthroughWalkthroughThe PR adds the retired DAPCache3 singleton cache with filesystem locking, size accounting, purge behavior, and tests. It also updates CI, coverage settings, macOS and MidnightBSD configuration, gcovr setup, and disables DAPCache3Test build integration. ChangesDAPCache3 cache
Platform and CI configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Will this bring the numbers for coverage we see on ubuntu and in sonarscan?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@retired/DAPCache3.cc`:
- Around line 175-185: Update DAPCache3::m_record_descriptor, both
unlock_and_close overloads, and m_get_descriptor to keep descriptor-to-filename
bookkeeping synchronized: explicitly reject duplicate descriptor or filename
mappings, erase the corresponding mapping whenever unlocking by descriptor or
filename, and validate that filename lookup succeeds before dereferencing the
iterator to prevent unknown filenames from closing unrelated files.
- Around line 914-944: Update the purge flow around get_cache_size() so it does
not acquire or release a read lock while the write lock remains held. Read the
current cache size under the existing write lock, validate that it is at least
size before subtracting, and update cache metadata while synchronization is
still guaranteed.
- Around line 546-558: The cache-control locks in DAPCache3::get_read_lock and
the corresponding write-lock acquisition path must be exception-safe. In
retired/DAPCache3.cc lines 546-558, ensure unlock_cache() runs when
getSharedLock() or m_record_descriptor() throws; apply the same protected
cleanup in lines 573-585 for createLockedFile() or descriptor recording,
preserving normal status returns.
- Around line 346-365: Update createLockedFile so that an fcntl locking failure
closes the descriptor and removes the newly created file with its file_name
before throwing InternalErr; preserve the existing EEXIST behavior and error
reporting.
- Around line 775-801: The directory scan in the cache-entry collection loop
must only accept filenames beginning with d_prefix followed by the generated
entry delimiter “#”. Update the condition around dirEntry and d_prefix so
unrelated same-prefix files such as metadata or shared-directory files are
excluded from files and cannot be purged.
In `@retired/DAPCache3.h`:
- Around line 118-123: Resolve the cache-size unit contract consistently: in
retired/DAPCache3.h lines 118-123, document get_instance’s size argument as
megabytes or preserve the bytes documentation and adjust behavior accordingly;
in retired/DAPCache3.cc lines 419-425, remove byte-to-megabyte conversion for
byte semantics or implement overflow-safe megabyte conversion and integer
capacity calculation; in retired/unit-tests/DAPCache3Test.cc line 74, pass a
value using the selected unit and assert the corresponding cache capacity.
In `@retired/unit-tests/DAPCache3Test.cc`:
- Around line 71-75: Update DAPCache3Test::setUp so all fixture instances reuse
one shared, stable cache directory when calling DAPCache3::get_instance, rather
than storing the directory only in the per-fixture d_cache_dir. Ensure
subsequent tests compare against the same directory retained by the singleton.
In `@unit-tests/CMakeLists.txt`:
- Line 24: The DAPCache3Test is missing from both supported test build systems.
In unit-tests/CMakeLists.txt at lines 24 and 76-77, restore the DAPCache3Test
target using the source under retired/unit-tests, compile/link
retired/DAPCache3.cc, and expose the retired include directory; in
unit-tests/Makefile.am at lines 92 and 188, restore DAPCache3Test in UNIT_TESTS
and define its sources with the correct retired paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 604681e5-8292-48c8-928d-1525b989c5b7
📒 Files selected for processing (11)
.github/workflows/cmake.yml.gitignore.travis.ymlconf/config.rpathgcovr.configretired/DAPCache3.ccretired/DAPCache3.hretired/unit-tests/DAPCache3Test.ccsonar-project.propertiesunit-tests/CMakeLists.txtunit-tests/Makefile.am
💤 Files with no reviewable changes (1)
- .github/workflows/cmake.yml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@retired/DAPCache3.cc`:
- Around line 175-185: Update DAPCache3::m_record_descriptor, both
unlock_and_close overloads, and m_get_descriptor to keep descriptor-to-filename
bookkeeping synchronized: explicitly reject duplicate descriptor or filename
mappings, erase the corresponding mapping whenever unlocking by descriptor or
filename, and validate that filename lookup succeeds before dereferencing the
iterator to prevent unknown filenames from closing unrelated files.
- Around line 914-944: Update the purge flow around get_cache_size() so it does
not acquire or release a read lock while the write lock remains held. Read the
current cache size under the existing write lock, validate that it is at least
size before subtracting, and update cache metadata while synchronization is
still guaranteed.
- Around line 546-558: The cache-control locks in DAPCache3::get_read_lock and
the corresponding write-lock acquisition path must be exception-safe. In
retired/DAPCache3.cc lines 546-558, ensure unlock_cache() runs when
getSharedLock() or m_record_descriptor() throws; apply the same protected
cleanup in lines 573-585 for createLockedFile() or descriptor recording,
preserving normal status returns.
- Around line 346-365: Update createLockedFile so that an fcntl locking failure
closes the descriptor and removes the newly created file with its file_name
before throwing InternalErr; preserve the existing EEXIST behavior and error
reporting.
- Around line 775-801: The directory scan in the cache-entry collection loop
must only accept filenames beginning with d_prefix followed by the generated
entry delimiter “#”. Update the condition around dirEntry and d_prefix so
unrelated same-prefix files such as metadata or shared-directory files are
excluded from files and cannot be purged.
In `@retired/DAPCache3.h`:
- Around line 118-123: Resolve the cache-size unit contract consistently: in
retired/DAPCache3.h lines 118-123, document get_instance’s size argument as
megabytes or preserve the bytes documentation and adjust behavior accordingly;
in retired/DAPCache3.cc lines 419-425, remove byte-to-megabyte conversion for
byte semantics or implement overflow-safe megabyte conversion and integer
capacity calculation; in retired/unit-tests/DAPCache3Test.cc line 74, pass a
value using the selected unit and assert the corresponding cache capacity.
In `@retired/unit-tests/DAPCache3Test.cc`:
- Around line 71-75: Update DAPCache3Test::setUp so all fixture instances reuse
one shared, stable cache directory when calling DAPCache3::get_instance, rather
than storing the directory only in the per-fixture d_cache_dir. Ensure
subsequent tests compare against the same directory retained by the singleton.
In `@unit-tests/CMakeLists.txt`:
- Line 24: The DAPCache3Test is missing from both supported test build systems.
In unit-tests/CMakeLists.txt at lines 24 and 76-77, restore the DAPCache3Test
target using the source under retired/unit-tests, compile/link
retired/DAPCache3.cc, and expose the retired include directory; in
unit-tests/Makefile.am at lines 92 and 188, restore DAPCache3Test in UNIT_TESTS
and define its sources with the correct retired paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 604681e5-8292-48c8-928d-1525b989c5b7
📒 Files selected for processing (11)
.github/workflows/cmake.yml.gitignore.travis.ymlconf/config.rpathgcovr.configretired/DAPCache3.ccretired/DAPCache3.hretired/unit-tests/DAPCache3Test.ccsonar-project.propertiesunit-tests/CMakeLists.txtunit-tests/Makefile.am
💤 Files with no reviewable changes (1)
- .github/workflows/cmake.yml
🛑 Comments failed to post (8)
retired/DAPCache3.cc (5)
175-185: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep descriptor bookkeeping synchronized across both unlock APIs.
unlock_and_close(int)leaves a stale filename mapping, duplicateinsert()calls are ignored, and an unknown filename dereferencesd_locks.end(). A reused descriptor can consequently close an unrelated file.Track and erase mappings for both overloads, reject duplicate locks explicitly, and validate lookup success.
Also applies to: 666-680
🧰 Tools
🪛 Clang (14.0.6)
[warning] 175-175: parameter name 'fd' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 180-180: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 180-180: method 'm_get_descriptor' can be made static
(readability-convert-member-functions-to-static)
[warning] 181-181: variable 'i' is not initialized
(cppcoreguidelines-init-variables)
[warning] 181-181: variable name 'i' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 182-182: variable 'fd' is not initialized
(cppcoreguidelines-init-variables)
[warning] 182-182: variable name 'fd' is too short, expected at least 3 characters
(readability-identifier-length)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@retired/DAPCache3.cc` around lines 175 - 185, Update DAPCache3::m_record_descriptor, both unlock_and_close overloads, and m_get_descriptor to keep descriptor-to-filename bookkeeping synchronized: explicitly reject duplicate descriptor or filename mappings, erase the corresponding mapping whenever unlocking by descriptor or filename, and validate that filename lookup succeeds before dereferencing the iterator to prevent unknown filenames from closing unrelated files.
346-365: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the newly created file when lock acquisition fails.
After
open(O_CREAT | O_EXCL)succeeds, anfcntl()failure leaves an unlocked empty file behind. Subsequentcreate_and_lock()calls then permanently returnfalse.Proposed cleanup
if (fcntl(fd, F_SETLKW, l) == -1) { + int saved_errno = errno; close(fd); + unlink(file_name.c_str()); + errno = saved_errno; ostringstream oss;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.static bool createLockedFile(string file_name, int &ref_fd) { DBG(cerr << "createLockedFile: " << file_name << endl); int fd; if ((fd = open(file_name.c_str(), O_CREAT | O_EXCL | O_RDWR, 0666)) < 0) { switch (errno) { case EEXIST: return false; default: throw InternalErr(__FILE__, __LINE__, get_errno()); } } struct flock *l = lock(F_WRLCK); if (fcntl(fd, F_SETLKW, l) == -1) { int saved_errno = errno; close(fd); unlink(file_name.c_str()); errno = saved_errno; ostringstream oss; oss << "cache process: " << l->l_pid << " triggered a locking error: " << get_errno(); throw InternalErr(__FILE__, __LINE__, oss.str());🧰 Tools
🪛 Clang (14.0.6)
[warning] 346-346: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 349-349: variable 'fd' is not initialized
(cppcoreguidelines-init-variables)
[warning] 349-349: variable name 'fd' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 360-360: variable name 'l' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 361-361: do not call c-style vararg functions
(cppcoreguidelines-pro-type-vararg)
[warning] 363-363: variable 'oss' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.21.0)
[performance] 346-346: Function parameter 'file_name' should be passed by const reference.
(passedByValue)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@retired/DAPCache3.cc` around lines 346 - 365, Update createLockedFile so that an fcntl locking failure closes the descriptor and removes the newly created file with its file_name before throwing InternalErr; preserve the existing EEXIST behavior and error reporting.
546-558: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Make cache-control locking exception-safe in both file acquisition paths.
retired/DAPCache3.cc#L546-L558: release the read lock ifgetSharedLock()or descriptor recording throws.retired/DAPCache3.cc#L573-L585: release the write lock ifcreateLockedFile()or descriptor recording throws.🧰 Tools
🪛 Clang (14.0.6)
[warning] 546-546: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 546-546: parameter name 'fd' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 553-553: statement should be inside braces
(readability-braces-around-statements)
📍 Affects 1 file
retired/DAPCache3.cc#L546-L558(this comment)retired/DAPCache3.cc#L573-L585🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@retired/DAPCache3.cc` around lines 546 - 558, The cache-control locks in DAPCache3::get_read_lock and the corresponding write-lock acquisition path must be exception-safe. In retired/DAPCache3.cc lines 546-558, ensure unlock_cache() runs when getSharedLock() or m_record_descriptor() throws; apply the same protected cleanup in lines 573-585 for createLockedFile() or descriptor recording, preserving normal status returns.
775-801: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restrict purging to actual cache-entry filenames.
The scan matches
d_prefix, but generated entries start withd_prefix + '#'. A prefix such asdapincludesdap.cache.info, and any same-prefix file in a shared directory can be deleted.+ const string entry_prefix = d_prefix + DAP_CACHE_CHAR; while ((dit = readdir(dip)) != NULL) { string dirEntry = dit->d_name; - if (dirEntry.compare(0, d_prefix.length(), d_prefix) == 0) { + if (dirEntry.compare(0, entry_prefix.length(), entry_prefix) == 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// go through the cache directory and collect all of the files that // start with the matching prefix const string entry_prefix = d_prefix + DAP_CACHE_CHAR; while ((dit = readdir(dip)) != NULL) { string dirEntry = dit->d_name; if (dirEntry.compare(0, entry_prefix.length(), entry_prefix) == 0) { files.push_back(d_cache_dir + "/" + dirEntry); } } closedir(dip); unsigned long long current_size = 0; struct stat buf; for (vector<string>::iterator file = files.begin(); file != files.end(); ++file) { if (stat(file->c_str(), &buf) == 0) { current_size += buf.st_size; cache_entry entry; entry.name = *file; entry.size = buf.st_size; entry.time = buf.st_atime; // Sanity check; Removed after initial testing since some files might be zero bytes `#if` 0 if (entry.size == 0) throw InternalErr(__FILE__, __LINE__, "Zero-byte file found in cache. " + *file); `#endif` contents.push_back(entry); }🧰 Tools
🪛 Clang (14.0.6)
[warning] 777-777: use nullptr
(modernize-use-nullptr)
[warning] 787-787: uninitialized record type: 'buf'
(cppcoreguidelines-pro-type-member-init)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@retired/DAPCache3.cc` around lines 775 - 801, The directory scan in the cache-entry collection loop must only accept filenames beginning with d_prefix followed by the generated entry delimiter “#”. Update the condition around dirEntry and d_prefix so unrelated same-prefix files such as metadata or shared-directory files are excluded from files and cannot be purged.
914-944: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not call
get_cache_size()while holding the write lock.POSIX record locks are process-associated:
get_cache_size()converts the existing write lock to a read lock and then releases it. Lines 937–941 consequently update cache metadata without synchronization. Read and subtract under the existing write lock, with an underflow check.🧰 Tools
🪛 Clang (14.0.6)
[warning] 918-918: variable 'cfile_fd' is not initialized
(cppcoreguidelines-init-variables)
[warning] 922-922: uninitialized record type: 'buf'
(cppcoreguidelines-pro-type-member-init)
[warning] 929-929: statement should be inside braces
(readability-braces-around-statements)
[warning] 937-937: statement should be inside braces
(readability-braces-around-statements)
[warning] 940-940: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@retired/DAPCache3.cc` around lines 914 - 944, Update the purge flow around get_cache_size() so it does not acquire or release a read lock while the write lock remains held. Read the current cache size under the existing write lock, validate that it is at least size before subtracting, and update cache metadata while synchronization is still guaranteed.retired/DAPCache3.h (1)
118-123: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the cache-size unit contract across the API, implementation, and test. The public API says bytes, while initialization and its constants use megabytes.
retired/DAPCache3.h#L118-L123: document the argument as megabytes, or retain bytes and make the implementation honor that contract.retired/DAPCache3.cc#L419-L425: remove the conversion for byte input, or use an overflow-safe megabyte cap and integer target calculation.retired/unit-tests/DAPCache3Test.cc#L74-L74: pass a value matching the chosen unit and assert the resulting capacity behavior.📍 Affects 3 files
retired/DAPCache3.h#L118-L123(this comment)retired/DAPCache3.cc#L419-L425retired/unit-tests/DAPCache3Test.cc#L74-L74🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@retired/DAPCache3.h` around lines 118 - 123, Resolve the cache-size unit contract consistently: in retired/DAPCache3.h lines 118-123, document get_instance’s size argument as megabytes or preserve the bytes documentation and adjust behavior accordingly; in retired/DAPCache3.cc lines 419-425, remove byte-to-megabyte conversion for byte semantics or implement overflow-safe megabyte conversion and integer capacity calculation; in retired/unit-tests/DAPCache3Test.cc line 74, pass a value using the selected unit and assert the corresponding cache capacity.retired/unit-tests/DAPCache3Test.cc (1)
71-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does CppUnit create a separate TestFixture instance for each test registered with CPPUNIT_TEST?💡 Result:
Yes, CppUnit is designed to run each test in its own separate TestFixture instance [1][2][3]. This ensures that tests are isolated and that there are no side effects between test runs [1][4][5]. When you use the CPPUNIT_TEST macro within a test suite, you are registering a test method to be executed [6][7]. Internally, CppUnit utilizes a TestCaller (or a similar mechanism, often via the HelperMacros) to generate a test case from the fixture method [6][7][8]. The TestCaller is responsible for creating a new instance of the TestFixture, calling the setUp method, running the specific test method, calling the tearDown method, and then destroying the fixture instance [1][2][8]. This behavior is fundamental to CppUnit's architecture, allowing each test to start with a fresh environment as defined in the fixture's setUp method [1][7][3].
Citations:
- 1: https://cppunit.sourceforge.net/doc/1.11.6/class_test_fixture.html
- 2: https://cppunit.sourceforge.net/doc/cvs/class_test_fixture.html
- 3: https://cppunit.sourceforge.net/doc/lastest/class_test_fixture.html
- 4: https://stackoverflow.com/questions/8999790/cppunit-how-to-create-a-instance-of-testfixture-using-its-name
- 5: https://stackoverflow.com/questions/10302054/cppunit-setup-and-teardown
- 6: https://cppunit.sourceforge.net/doc/1.11.6/group___writing_test_fixture.html
- 7: https://cppunit.sourceforge.net/doc/1.11.6/cppunit_cookbook.html
- 8: https://cppunit.sourceforge.net/doc/1.11.6/class_test_caller.html
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the test and cache implementation. git ls-files | rg '(^|/)DAPCache3Test\.cc$|(^|/)DAPCache3\.(cc|h|hh|hpp)$|(^|/)DAPCache3.*'Repository: OPENDAP/libdap4
Length of output: 230
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files | rg '(^|/)DAPCache3Test\.cc$|(^|/)DAPCache3\.(cc|h|hh|hpp)$|(^|/)DAPCache3.*'Repository: OPENDAP/libdap4
Length of output: 230
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline retired/unit-tests/DAPCache3Test.cc --view expanded || true printf '\n---\n' ast-grep outline retired/DAPCache3.cc --view expanded || true printf '\n---\n' sed -n '1,220p' retired/unit-tests/DAPCache3Test.cc printf '\n---\n' sed -n '1,260p' retired/DAPCache3.ccRepository: OPENDAP/libdap4
Length of output: 15157
Use a shared cache directory for this singleton test setup. CppUnit creates a fresh fixture per test, so
d_cache_dirchanges on each run whileDAPCache3::get_instance()keeps the first directory. The later test will compare against a different path than the singleton actually uses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@retired/unit-tests/DAPCache3Test.cc` around lines 71 - 75, Update DAPCache3Test::setUp so all fixture instances reuse one shared, stable cache directory when calling DAPCache3::get_instance, rather than storing the directory only in the per-fixture d_cache_dir. Ensure subsequent tests compare against the same directory retained by the singleton.unit-tests/CMakeLists.txt (1)
24-24: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The new
DAPCache3Testis disabled in both supported test build systems, defeating the PR’s coverage objective.
unit-tests/CMakeLists.txt#L24-L24: restore a test target for the source underretired/unit-tests.unit-tests/CMakeLists.txt#L76-L77: compile/linkretired/DAPCache3.ccand expose theretiredinclude directory.unit-tests/Makefile.am#L92-L92: restoreDAPCache3TesttoUNIT_TESTS.unit-tests/Makefile.am#L188-L188: restore its source definition with the correct retired paths.📍 Affects 2 files
unit-tests/CMakeLists.txt#L24-L24(this comment)unit-tests/CMakeLists.txt#L76-L77unit-tests/Makefile.am#L92-L92unit-tests/Makefile.am#L188-L188🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@unit-tests/CMakeLists.txt` at line 24, The DAPCache3Test is missing from both supported test build systems. In unit-tests/CMakeLists.txt at lines 24 and 76-77, restore the DAPCache3Test target using the source under retired/unit-tests, compile/link retired/DAPCache3.cc, and expose the retired include directory; in unit-tests/Makefile.am at lines 92 and 188, restore DAPCache3Test in UNIT_TESTS and define its sources with the correct retired paths.
|
|
I don't know if you picker up on this, but CodeRabbit analyzed the DAPCache code and found some issues there. For example:
That's pretty impressive if it's true. This code is retired, but still it's interesting that the analysis was so deep. Sonar never said a word about this. (of course, I need to change CodeRabbit to ignore stuff in |
| - os: ubuntu-latest | ||
| name: Linux | ||
| - os: macos-15 | ||
| name: macOS 15 arm64 |
3 participants
The libdap4 repo has broken test coverage information; fix
This library is the core of our server, in one sense at least, and the reference implementation for the DAP protocol. If we want to improve test coverage in this code, we need to measure the coverage accurately. There are three sources of information regarding coverage, and all three show very different figures. On Mac OS I see ~60% coverage, on Ubuntu, less (~54%) and on SonarScan even less (41%). Same code, same tests.
Also, we use this repo as a kind of guinea pig for the BES, where the build is far more complex. Once we get this sorted here, we can duplicate it for the BES
GH Issue #362, #367
Update 7/17/26
I used Claude code to work this, partly to form an example of how to use a tool like that with a problem in existing legacy code.
Background
To generate coverage data for our tests, we compile the code using special options that instrument the code and then run the tests. The data files written are then processed by a tool to build 'coverage reports.' The tool is called
gcovr.Solution
On macos I was using gcovr version 8.6, on Ubuntu (focal) the default version installed is version 4.2. That was the core issue. Version 4.2 of gcovr has bugs and I needed to force the Travis builds to use a newer version. I settled on 8.2 since that is the newest version that will work on Ubuntu focal.
Why the issue with SonarScan? More Background: SonarScan reports coverage data using a 'report' we upload to it from the Travis build. Travis uses Ubuntu focal and thus was using gcovr 4.2. One bug 4.2 had was to make a report that sonarscan could not parse correctly, so that probably explains the discrepancy between the Ubuntu and Sonar numbers. For this, I cannot tell until we merge this branch to master because that's when we get a scan report on all the software.
Also, I fixed #367 because removing DAPCache3 broke the CMake builds (the github workflows).