Skip to content

refactor: S24.10 rename file→block in BlockStore test names and comments#452

Merged
DavidCozens merged 1 commit into
mainfrom
refactor/s24-10-block-test-naming
May 25, 2026
Merged

refactor: S24.10 rename file→block in BlockStore test names and comments#452
DavidCozens merged 1 commit into
mainfrom
refactor/s24-10-block-test-naming

Conversation

@DavidCozens

Copy link
Copy Markdown
Owner

Summary

The BlockStore tests leaked file/File naming into test identifiers and block-index comments, conflating the BlockStore abstraction (over an injected BlockDevice) with the FileBlockDevice concrete adapter. Production code was already clean; this is a test-only rename.

  • BlockStoreTest.cpp — 13 test names renamed (WriteRotatesToNewFileWhenFull…NewBlockWhenFull, MaxFilesAtUpperLimitMaxBlocksAtUpperLimit, etc.), ~30 /* file NN */ block-index comments changed to /* block NN */, and the "File rotation" / "Resume from existing file" section headers renamed.
  • BlockStorePosixTest.cppDiscardOldestWhenReadIsPartwayThroughOldestFile…OldestBlock, plus 10 block-index comments.
  • BlockStoreDrainOrderingTest.cpp — fixture comment header reworded.

Deliberate non-changes

The story body suggested renaming the fixture member filedevice/blockDevice. On close inspection that's not workable:

  • device is already a member (the SolidSyslogBlockDevice*); renaming the file member to device would collide.
  • file is genuinely a SolidSyslogFile* — used by many tests for file-layer assertions (SolidSyslogFile_Exists(file, ...)) and fault injection (FileFake_FailNextOpen(file), FileFake_FailNextWrite(file), etc.). The right name for a SolidSyslogFile* variable is file.

The fixture comment is extended to make that intent explicit. Test bodies that assert file-layer side effects of BlockStore operations continue to use file correctly.

The PosixTest doc-comment about "real POSIX files" and the CleanStoreFiles() helper are also kept — that integration test genuinely is about files on disk.

Test plan

  • cmake --preset debug && cmake --build --preset debug --target junit — 1332 tests pass
  • Renamed tests confirmed running green (MaxBlocksAtUpperLimit, WriteRotatesToNewBlockWhenFull, CreatesFirstBlockOnInit, etc.)
  • clang-format -i applied to touched files
  • cmake --build --preset cppcheck — green (no new MISRA findings from rename)
  • No external line-anchored cppcheck/MISRA suppressions reference these files (only inline // cppcheck-suppress comments which move with their line)

Closes #449

The BlockStore tests leaked `file`/`File` naming into test identifiers
and block-index comments, conflating the BlockStore abstraction (over
an injected BlockDevice) with the `FileBlockDevice` concrete adapter.
Production code was already clean; this only touches three test files.

- BlockStoreTest.cpp: 13 test names renamed (e.g.
  WriteRotatesToNewFileWhenFull → …NewBlockWhenFull, MaxFilesAtUpperLimit
  → MaxBlocksAtUpperLimit), ~30 `/* file NN */` block-index comments
  changed to `/* block NN */`, and the "File rotation" /
  "Resume from existing file" section headers renamed.

- BlockStorePosixTest.cpp: DiscardOldestWhenReadIsPartwayThroughOldestFile
  → …OldestBlock, plus 10 block-index comments.

- BlockStoreDrainOrderingTest.cpp: fixture comment header reworded.

The fixture members `file` (a `SolidSyslogFile*`) and `device` (the
`SolidSyslogBlockDevice*`) are kept as-is — `file` is genuinely a
SolidSyslogFile pointer used by many tests for file-layer assertions
(`SolidSyslogFile_Exists`) and fault injection (`FileFake_FailNextOpen`,
etc.), so the name is accurate. The fixture comment is extended to
make that intent explicit.

The PosixTest doc-comment "real POSIX files" and the `CleanStoreFiles()`
helper are kept — that integration test genuinely is about files on disk.

All 1332 tests pass under the debug preset.

Closes #449
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@DavidCozens, we couldn't start this review because you've used your available PR reviews for now.

Your plan includes 1 review of capacity. Refill in 36 minutes and 19 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0cc505f7-7950-47e4-aad4-18d6ff8141a4

📥 Commits

Reviewing files that changed from the base of the PR and between 5b38fd8 and db3bcdf.

📒 Files selected for processing (3)
  • Tests/SolidSyslogBlockStoreDrainOrderingTest.cpp
  • Tests/SolidSyslogBlockStorePosixTest.cpp
  • Tests/SolidSyslogBlockStoreTest.cpp
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/s24-10-block-test-naming

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

☀️   Quality Summary

   🚦   build-linux-gcc: 100% successful (✔️ 1338 passed)
   🚦   build-freertos-host-tdd: 100% successful (✔️ 1572 passed)
   🚦   build-linux-clang: 100% successful (✔️ 1290 passed)
   🚦   sanitize-linux-gcc: 100% successful (✔️ 1290 passed)
   🚦   integration-linux-openssl: 100% successful (✔️ 9 passed)
   🚦   integration-linux-mbedtls: 100% successful (✔️ 7 passed)
   🚦   integration-windows-openssl: 100% successful (✔️ 9 passed)
   🚦   bdd-linux-syslog-ng: 94% successful (✔️ 46 passed, 🙈 3 skipped)
   🚦   bdd-windows-otel: 90% successful (✔️ 44 passed, 🙈 5 skipped)
   🚦   bdd-freertos-qemu: 86% successful (✔️ 42 passed, 🙈 7 skipped)
   🚦   build-windows-msvc: 100% successful (✔️ 1149 passed)
   🚦   build-linux-tunable-override: 100% successful (✔️ 1290 passed)
   ⚠️   Clang-Tidy: No warnings
   ⚠️   CPPCheck: 1 warning (low: 1)


Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result.

@DavidCozens DavidCozens merged commit 1658a89 into main May 25, 2026
21 checks passed
@DavidCozens DavidCozens deleted the refactor/s24-10-block-test-naming branch May 25, 2026 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S24.10: rename file-leaking identifiers and comments in BlockStore tests

1 participant