Engraving tests: rewrite ScoreComp::compareFiles without diff#33822
Engraving tests: rewrite ScoreComp::compareFiles without diff#33822RomanPudashkin wants to merge 2 commits into
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/engraving/tests/utils/scorecomp.cpp (1)
55-56: 🏗️ Heavy liftThe LCS DP table is quadratic in memory and can blow up on larger files.
computeDiff()allocates(m+1)*(n+1)size_tentries; for larger score snapshots this can become hundreds of MB/GB and hurt CI reliability. Consider a linear-space diff strategy (or a size guard + fallback path).🤖 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 `@src/engraving/tests/utils/scorecomp.cpp` around lines 55 - 56, The `computeDiff()` function allocates a dp table with quadratic memory usage of (m+1)*(n+1) size_t entries, which can consume hundreds of MB/GB for large files and impact CI reliability. Refactor this to use a linear-space diff strategy such as Hirschberg's algorithm instead of the standard dynamic programming approach, or alternatively add a size guard that checks if m*n exceeds a reasonable threshold and falls back to a more memory-efficient diff algorithm when exceeded. Update the dp vector allocation and the LCS computation logic to use the optimized approach.
🤖 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 `@src/engraving/tests/utils/scorecomp.cpp`:
- Around line 210-216: The readLines() function masks file I/O errors by
returning an empty list instead of propagating the error to its caller. Change
the return type of readLines() from StringList to RetVal<StringList> so that
when the file read fails (when !rv.ret is true), the function propagates the
error via the RetVal wrapper rather than returning an empty list. This allows
compareFiles() to properly distinguish between truly empty files and files that
failed to read, preventing false-positive test results.
---
Nitpick comments:
In `@src/engraving/tests/utils/scorecomp.cpp`:
- Around line 55-56: The `computeDiff()` function allocates a dp table with
quadratic memory usage of (m+1)*(n+1) size_t entries, which can consume hundreds
of MB/GB for large files and impact CI reliability. Refactor this to use a
linear-space diff strategy such as Hirschberg's algorithm instead of the
standard dynamic programming approach, or alternatively add a size guard that
checks if m*n exceeds a reasonable threshold and falls back to a more
memory-efficient diff algorithm when exceeded. Update the dp vector allocation
and the LCS computation logic to use the optimized approach.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a674c25c-1105-448c-91fc-83f4f780bffd
📒 Files selected for processing (6)
src/engraving/tests/utils/scorecomp.cppsrc/engraving/tests/utils/scorecomp.hsrc/importexport/bb/tests/CMakeLists.txtsrc/importexport/bb/tests/testbase.cppsrc/importexport/bww/tests/CMakeLists.txtsrc/importexport/bww/tests/testbase.cpp
| StringList ScoreComp::readLines(const String& path) | ||
| { | ||
| RetVal<ByteArray> rv = fileSystem()->readFile(path); | ||
| if (!rv.ret) { | ||
| LOGE() << "Failed to read: " << path << ", err: " << rv.ret.toString(); | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
Propagate file-read failures instead of returning an empty line list.
readLines() currently returns {} on I/O error, which makes compareFiles() treat unreadable files as equal in some cases (e.g., both reads fail, or failed read vs truly empty file). This can produce false-positive passing tests now that bb/bww delegate to ScoreComp::compareFiles().
🤖 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 `@src/engraving/tests/utils/scorecomp.cpp` around lines 210 - 216, The
readLines() function masks file I/O errors by returning an empty list instead of
propagating the error to its caller. Change the return type of readLines() from
StringList to RetVal<StringList> so that when the file read fails (when !rv.ret
is true), the function propagates the error via the RetVal wrapper rather than
returning an empty list. This allows compareFiles() to properly distinguish
between truly empty files and files that failed to read, preventing
false-positive test results.
| QTextStream out(stdout); | ||
| out << p.readAllStandardOutput(); | ||
| err << p.readAllStandardError(); | ||
| StringList lines = String::fromUtf8(rv.val).split(Char(u'\n')); |
There was a problem hiding this comment.
Maybe it would be nice to avoid muse::String, to avoid the 8-bit -> 16-bit conversion, for another speed up
Could perhaps also use std::string_view, to avoid copying into the list of lines.
2 participants
Advantages: