Add comprehensive test coverage for CSVDialog class #582
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughTwo files modified: a robustness improvement to line-reading logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model/CSVDialog.cc(1 hunks)test/testCSVParser.cc(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 541
File: engine/CSVParser.cc:652-656
Timestamp: 2025-02-06T01:34:05.110Z
Learning: In the CSVParser, bad input files are handled gracefully by treating them as empty files, without throwing explicit errors. This is by design to make the parser more robust.
📚 Learning: 2025-02-06T01:34:05.110Z
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 541
File: engine/CSVParser.cc:652-656
Timestamp: 2025-02-06T01:34:05.110Z
Learning: In the CSVParser, bad input files are handled gracefully by treating them as empty files, without throwing explicit errors. This is by design to make the parser more robust.
Applied to files:
test/testCSVParser.cc
📚 Learning: 2024-12-30T09:34:38.295Z
Learnt from: highperformancecoder
Repo: highperformancecoder/minsky PR: 538
File: engine/variableValue.cc:331-351
Timestamp: 2024-12-30T09:34:38.295Z
Learning: The user prefers failing silently for dimension lookups in `variableValue.cc` rather than throwing an exception if a dimension is not found.
Applied to files:
test/testCSVParser.cc
🪛 Clang (14.0.6)
test/testCSVParser.cc
[error] 237-237: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 238-238: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 259-259: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 260-260: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 280-280: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 281-281: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 301-301: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 302-302: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 325-325: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 326-326: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 347-347: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 348-348: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 371-371: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 372-372: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 394-394: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 395-395: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 420-420: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 421-421: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 426-426: variable 'outputFile' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 444-444: variable 'outputFile' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 447-447: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 448-448: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 470-470: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 471-471: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 495-495: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 496-496: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 520-520: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 525-525: 150 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers,-warnings-as-errors)
[error] 539-539: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 540-540: variable 'input' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 605-605: variable 'expected' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 648-648: variable 'expected' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 764-764: variable 'expected' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 794-794: variable 'is' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 794-794: variable name 'is' is too short, expected at least 3 characters
(readability-identifier-length,-warnings-as-errors)
[error] 801-801: variable 'is' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 801-801: variable name 'is' is too short, expected at least 3 characters
(readability-identifier-length,-warnings-as-errors)
[error] 808-808: variable 'is' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 808-808: variable name 'is' is too short, expected at least 3 characters
(readability-identifier-length,-warnings-as-errors)
[error] 815-815: variable 'is' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 815-815: variable name 'is' is too short, expected at least 3 characters
(readability-identifier-length,-warnings-as-errors)
[error] 822-822: variable 'is' is not initialized
(cppcoreguidelines-init-variables,-warnings-as-errors)
[error] 822-822: variable name 'is' is too short, expected at least 3 characters
(readability-identifier-length,-warnings-as-errors)
[error] 829-829: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
[error] 896-896: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (2)
test/testCSVParser.cc (2)
237-565: Excellent comprehensive test coverage for CSVDialog.The 14 new test cases provide thorough coverage of the CSVDialog class:
- Core functionality: file loading (including DOS line endings), automatic format detection, header population, various parsing modes (merge delimiters, max columns, escaped quotes), unique value calculation, and report generation
- Edge cases: invalid header row/column indices, large file truncation (>100 lines), empty column data, and special character handling
All tests follow the existing fixture pattern with proper resource cleanup and align well with the PR objectives.
592-910: Good practice: ASSERT to EXPECT conversions improve test diagnostics.Converting strict
ASSERT_*macros toEXPECT_*macros allows tests to continue execution and report multiple failures per test run, rather than stopping at the first failure. This improves test diagnostics and follows Google Test best practices.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for the CSVDialog class, increasing from 1 test to 15 tests and achieving 100% method coverage of the 10 public methods. The PR also includes a bug fix in model/CSVDialog.cc that prevents empty lines from being added to initialLines when getline() fails at the end of a file or on read errors.
Key changes:
- Added 14 new focused tests covering all CSVDialog public methods including edge cases
- Fixed a subtle bug in
loadFileFromName()where failedgetline()calls could add empty strings to the initial lines buffer - Converted many
ASSERT_*macros toEXPECT_*in existing tests where appropriate (keepingASSERT_*for structural checks that could cause undefined behavior)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/testCSVParser.cc | Added 14 comprehensive tests for CSVDialog methods (file loading, parsing, header population, classification, edge cases); converted ASSERT_* to EXPECT_* for non-critical assertions in existing tests |
| model/CSVDialog.cc | Fixed bug in loadFileFromName() to prevent adding empty lines when getline() fails by checking the return value before emplacing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dialog.populateHeaders(); | ||
|
|
||
| // Headers should not have been populated | ||
| EXPECT_TRUE(spec.dimensionNames.empty() || spec.dimensionNames.size() == 3); |
There was a problem hiding this comment.
[nitpick] The assertion EXPECT_TRUE(spec.dimensionNames.empty() || spec.dimensionNames.size() == 3) is too weak and doesn't properly verify the test's purpose.
Looking at the populateHeaders() implementation, when headerRow is invalid (>= parsedLines.size()), it returns early without modifying dimensionNames. Therefore, the state of dimensionNames depends on what happened before this test runs.
The test should instead verify the expected behavior more explicitly. Consider either:
- Initializing
dimensionNamesto a known state before callingpopulateHeaders(), then verifying it remains unchanged - Checking that
dimensionNamesis empty (if that's the expected initial state)
As written, this assertion would pass even if populateHeaders() incorrectly modified dimensionNames to have 3 elements.
|
|
||
| // Empty columns should be treated as 'ignore' | ||
| ASSERT_EQ(3, spec.numCols); | ||
| // None of the three columns should be in dataCols, nor dimensionCols since they're empty |
There was a problem hiding this comment.
The comment has an extra space: "None of the three columns" should be "None of the three columns" (remove one space between "the" and "three").
| // None of the three columns should be in dataCols, nor dimensionCols since they're empty | |
| // None of the three columns should be in dataCols, nor dimensionCols since they're empty |
The CSVDialog class in
model/CSVDialog.cchad minimal test coverage with only one test (ClassifyColumns). This adds 14 targeted tests to achieve 100% method coverage.Changes
Core method tests (9 tests)
loadFile()/loadFileFromName()- file loading with DOS line ending handlingguessSpecAndLoadFile()- automatic CSV format detectionpopulateHeaders()/populateHeader()- header name extractionparseLines()- delimiter modes (merged, quoted, column limits)correctedUniqueValues()- unique value calculation with header correctionreportFromFile()- CSV report generationEdge case tests (5 tests)
All tests use temporary files with proper cleanup and follow existing test patterns. The
isNumerical()helper function already had comprehensive coverage.Test coverage: 1 → 15 tests, 10/10 public methods covered
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
esm.ubuntu.com/usr/lib/apt/methods/https(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
This change is
Summary by CodeRabbit