Repository navigation
Convert remaining UnitTest++ test files to GTest - #566
highperformancecoder merged 14 commits into
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR centralizes macro cleanup and includes in minsky_epilogue.h, removes redundant undefines from model/cairoItems.h, and migrates the test suite from UnitTest++ to Google Test. It also restructures the test build: replaces the old test runner and objects, deletes several legacy tests, adjusts shell/Python test harnesses, and updates Makefile targets. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
…e, testSaver, testPlotWidget, testPhillips) Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
…file Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
| CHECK_EQUAL(group0AccessibleVars.size(), a->variableCast()->accessibleVars().size()); | ||
| CHECK_ARRAY_EQUAL(group0AccessibleVars, a->variableCast()->accessibleVars(), group0AccessibleVars.size()); | ||
| EXPECT_EQ(globalAccessibleVars.size(), model->accessibleVars().size()); | ||
| for (size_t _i=0; _i<globalAccessibleVars.size(; ++_i) EXPECT_EQ(globalAccessibleVars[_i], model->accessibleVars()[_i])); |
There was a problem hiding this comment.
This loop and next two look they wouldn't compile
There was a problem hiding this comment.
Fixed in 92e3f27. The Python conversion script incorrectly parsed the CHECK_ARRAY_EQUAL macros, resulting in malformed for loop conditions with missing closing parentheses (e.g., .size(; instead of .size();).
| CHECK_ARRAY_EQUAL(expected,gathered.begin(),expected.size()); | ||
| CHECK_ARRAY_EQUAL(gathered.index(), toVal.index(), toVal.index().size()); | ||
| EXPECT_EQ(expected.size(), gathered.size()); | ||
| for (size_t _i=0; _i<expected.size(; ++_i) EXPECT_EQ(expected[_i], gathered.begin()[_i])); |
There was a problem hiding this comment.
Fixed in 92e3f27. Removed extra closing parenthesis and corrected the for loop condition.
| CHECK_ARRAY_EQUAL(gathered.index(), toVal.index(), toVal.index().size()); | ||
| EXPECT_EQ(expected.size(), gathered.size()); | ||
| for (size_t _i=0; _i<expected.size(; ++_i) EXPECT_EQ(expected[_i], gathered.begin()[_i])); | ||
| for (size_t _i=0; _i<toVal.index(; ++_i) EXPECT_EQ(gathered.index()[_i], toVal.index()[_i]).size()); |
There was a problem hiding this comment.
Fixed in 92e3f27. Corrected the malformed .index(; to .index().size(); and restructured the line properly.
|
@copilot fix syntax errors |
- Fixed malformed function calls ending with (; in for loop conditions - Removed extra closing parentheses from single-line for loops - Fixed .size(; -> .size(); and .rank(; -> .rank(); - Fixed special case in testTensorOps.cc line 591 with .index(; Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
All syntax errors have been fixed in 92e3f27. The issues were:
|
|
@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: 7
🧹 Nitpick comments (2)
test/testVariablePane.cc (2)
46-46: Consider usingEXPECT_LEfor clearer intent.The assertion
EXPECT_TRUE(nonTempCount<=variablePane.numRows()*variablePane.numCols())is correct, butEXPECT_LE(nonTempCount, variablePane.numRows()*variablePane.numCols())would be more idiomatic and produce better failure messages showing both values.Apply this diff:
- EXPECT_TRUE(nonTempCount<=variablePane.numRows()*variablePane.numCols()); + EXPECT_LE(nonTempCount, variablePane.numRows()*variablePane.numCols());
62-62: Consider usingEXPECT_GTfor clearer intent.Similar to line 46,
EXPECT_TRUE(variablePane.cell(0,0).width()>0)works butEXPECT_GT(variablePane.cell(0,0).width(), 0)is more idiomatic GoogleTest style and produces better failure diagnostics.Apply this diff:
- EXPECT_TRUE(variablePane.cell(0,0).width()>0); + EXPECT_GT(variablePane.cell(0,0).width(), 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
minsky_epilogue.h(2 hunks)model/cairoItems.h(0 hunks)test/00/t0022a.sh(1 hunks)test/Makefile(3 hunks)test/common-test.sh(1 hunks)test/main.cc(0 hunks)test/testCSVParser.cc(1 hunks)test/testCSVParserGemini.cc(0 hunks)test/testCSVParserGtest.cc(0 hunks)test/testDatabase.cc(0 hunks)test/testDerivative.cc(2 hunks)test/testExpressionWalker.cc(6 hunks)test/testGrid.cc(3 hunks)test/testLatexToPango.cc(2 hunks)test/testLockGroup.cc(2 hunks)test/testMdl.cc(1 hunks)test/testMinsky.cc(41 hunks)test/testPannableTab.cc(2 hunks)test/testPhillips.cc(1 hunks)test/testPlotWidget.cc(3 hunks)test/testPubTab.cc(1 hunks)test/testRESTService.cc(0 hunks)test/testSaver.cc(1 hunks)test/testStr.cc(1 hunks)test/testUnits.cc(9 hunks)test/testUserFunction.cc(2 hunks)test/testVariable.cc(1 hunks)test/testVariablePane.cc(1 hunks)test/testXVector.cc(2 hunks)test/ticket-1461.cc(1 hunks)
💤 Files with no reviewable changes (6)
- test/testRESTService.cc
- test/main.cc
- model/cairoItems.h
- test/testDatabase.cc
- test/testCSVParserGtest.cc
- test/testCSVParserGemini.cc
🧰 Additional context used
🪛 checkmake (0.2.2)
test/Makefile
[warning] 17-17: Target "FLAGS" should be declared PHONY.
(phonydeclared)
[warning] 45-45: Target "all" should be declared PHONY.
(phonydeclared)
⏰ 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 (14)
test/common-test.sh (1)
30-30: LGTM! Shell variable quoting improvement.Adding quotes around
$here:$PYTHONPATHprevents word splitting and is a defensive shell scripting best practice.test/testLatexToPango.cc (3)
20-20: LGTM! Test framework migration.Successfully migrated from UnitTest++ to GoogleTest.
25-25: LGTM! Test structure updated correctly.The test macro conversion from single-parameter
TEST(LaTeXToPango)to two-parameterTEST(LaTeXToPango, LaTeXToPango)follows GoogleTest conventions.
56-56: LGTM! Assertion macro updated correctly.The conversion from
CHECK_EQUALtoEXPECT_EQis correct per the documented macro mappings.test/testGrid.cc (3)
51-51: LGTM! Fixture correctly updated for GoogleTest.The
EvenHeightLeftGridfixture now inherits from::testing::Test, which is the correct pattern for GoogleTest fixtures.
57-57: LGTM! Fixture correctly updated for GoogleTest.The
UnevenHeightGridMockfixture now inherits from::testing::Test, following GoogleTest conventions.
77-208: LGTM! All tests successfully converted to GoogleTest.All test declarations have been correctly converted from
TEST_FIXTUREtoTEST_F, and the test logic remains intact. The conversions follow the documented migration patterns.test/00/t0022a.sh (2)
4-4: LGTM! Centralized test infrastructure.Sourcing the common test script provides standardized setup, cleanup, and error handling across the test suite.
7-33: LGTM! Python-based test approach.The test has been modernized to use Python scripts with the pyminsky module. The assertions test tooltip and detailedText functionality correctly, and the reload verification ensures persistence works as expected.
test/testUserFunction.cc (2)
28-32: LGTM! Fixture correctly designed for GoogleTest.The
UserFunctionSuitefixture inherits from bothMinsky(for test context) and::testing::Test(for GoogleTest integration), and correctly exposes theLocalMinskymember publicly for test access.
35-94: LGTM! Tests correctly converted to GoogleTest.The tests are appropriately split:
symbolNames1usesTEST(no fixture needed)- Other tests use
TEST_FwithUserFunctionSuite(require Minsky context)Assertion macros are correctly mapped (
CHECK_EQUAL→EXPECT_EQ,CHECK_THROW→EXPECT_THROW).test/testExpressionWalker.cc (2)
28-34: LGTM! Useful test helper function.The
checkCompilehelper centralizes expression compilation logic and provides useful diagnostics by outputting parser errors. This improves test maintainability.
36-213: LGTM! Tests successfully converted to GoogleTest.The test has been correctly converted:
- Test structure:
TEST(name)→TEST(ExpressionWalker, UnitsExpressionWalker)- Assertions:
CHECK_EQUAL→EXPECT_EQ,CHECK_THROW→EXPECT_THROW- Test logic preserved throughout
test/testVariablePane.cc (1)
27-34: LGTM! Fixture follows GoogleTest best practices.The fixture correctly inherits from both the domain class (
Minsky) and::testing::Test. TheLocalMinskymember is properly initialized with*this, establishing the correct ownership relationship. This pattern aligns with the migration strategy across the test suite.
|
|
||
| // these macros are defined in XLib, but interfere with gtest, for example. | ||
| #undef True | ||
| #undef Bool | ||
| #undef None |
There was a problem hiding this comment.
Move macro undefs inside the header guard.
The #undef directives for True, Bool, and None are placed after the closing #endif of the header guard (line 166). This means they will be processed every time this header is included, even on subsequent includes when the header guard prevents the main content from being processed. This could lead to unintended macro removal if these macros are defined by headers included after this one.
Apply this diff to move the undefs inside the header guard:
#endif
-
-// these macros are defined in XLib, but interfere with gtest, for example.
-#undef True
-#undef Bool
-#undef None
+
+// these macros are defined in XLib, but interfere with gtest, for example.
+#undef True
+#undef Bool
+#undef None
+
+#endifWait, let me reconsider. Looking at the line numbers again:
- Line 166 has
#endif - Lines 167-171 are the undefs
The undefs are currently AFTER the final #endif. They should be BEFORE it. Let me provide the correct diff:
+// these macros are defined in XLib, but interfere with gtest, for example.
+#undef True
+#undef Bool
+#undef None
+
#endif
-
-// these macros are defined in XLib, but interfere with gtest, for example.
-#undef True
-#undef Bool
-#undef None🤖 Prompt for AI Agents
In minsky_epilogue.h around lines 167 to 171, the #undef directives for True,
Bool, and None are placed after the header-guard closing #endif (line 166)
causing them to run on every inclusion; move those three #undef lines so they
appear just before the closing #endif of the header guard (i.e., inside the
guard) so the macros are only undefined when this header's contents are actually
processed.
There was a problem hiding this comment.
That is what I want - these macros should never be defined in the first place!
There was a problem hiding this comment.
@highperformancecoder, thank you for the clarification! I understand now – you want these macros unconditionally undefined on every inclusion to prevent XLib macro pollution from interfering with gtest and other code. The placement after the header guard achieves exactly that. My apologies for the confusion!
| } catch (const std::exception& ex) { | ||
| ASSERT_TRUE(string(ex.what()).starts_with("Invalid")); | ||
| } |
There was a problem hiding this comment.
Avoid C++20-only API in assertion
std::string::starts_with is a C++20 addition. The project still builds as C++17, so this line will not compile. Please rewrite the check using a C++17-friendly construct such as rfind("Invalid", 0) == 0.
- ASSERT_TRUE(string(ex.what()).starts_with("Invalid"));
+ const string msg = ex.what();
+ ASSERT_TRUE(msg.rfind("Invalid", 0) == 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.
| } catch (const std::exception& ex) { | |
| ASSERT_TRUE(string(ex.what()).starts_with("Invalid")); | |
| } | |
| } catch (const std::exception& ex) { | |
| const string msg = ex.what(); | |
| ASSERT_TRUE(msg.rfind("Invalid", 0) == 0); | |
| } |
🤖 Prompt for AI Agents
In test/testCSVParser.cc around lines 154 to 156, the assertion uses
std::string::starts_with which is C++20-only and fails under the project's C++17
build; replace that call with a C++17-compatible check such as testing that
rfind("Invalid", 0) == 0 (or comparing substr(0, N) to "Invalid") so the
assertion verifies the message prefix without using C++20 APIs.
There was a problem hiding this comment.
Thanks - I'll change the build to specify c++-20
There was a problem hiding this comment.
@highperformancecoder, sounds good! Upgrading the build to C++20 will resolve this issue and unlock other modern C++ features as well. Once you update the build configuration, std::string::starts_with and other C++20 APIs will work without modification.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actually it already does - see test/Makefile:18.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check test/Makefile around line 18 for C++ standard specification
head -n 25 test/Makefile | tail -n 15
Length of output: 1051
@highperformancecoder, you're absolutely right! I can confirm that test/Makefile line 18 does indeed specify -std=c++20:
FLAGS+=-std=c++20 -DJSON_PACK_NO_FALL_THROUGH_TO_STREAMING -DUSE_UNROLLED -DCLASSDESC_ARITIES=0xf \My apologies for the incorrect flag. Since the build is already configured for C++20, the use of std::string::starts_with in line 155 is perfectly valid and will compile without any issues. The original suggestion to rewrite it for C++17 compatibility was unnecessary.
🧬 Code Graph Analysis Results
-
File: model/CSVDialog.cc, lines 345-384
CSVDialog::classifyColumns()
{
auto parsedLines=parseLines();
spec.dimensionCols.clear();
spec.dataCols.clear();
spec.dimensions.resize(min(spec.numCols,spec.maxColumn));
for (size_t col=0; col<spec.numCols; ++col)
{
bool entryFound=false, timeFound=true, numberFound=true;
for (size_t row=spec.nRowAxes(); row<parsedLines.size(); ++row)
if (col<parsedLines[row].size() && !parsedLines[row][col].empty())
{
entryFound=true;
if (numberFound && !isNumerical(parsedLines[row][col]))
numberFound=false;
static const AnyVal any(Dimension(Dimension::time,""));
if (timeFound)
try
{any(parsedLines[row][col]);}
catch (...)
{timeFound=false;}
}
if (entryFound && col<spec.maxColumn)
{
if (numberFound)
spec.dataCols.insert(col);
else
{
spec.dimensionCols.insert(col);
if (timeFound)
spec.dimensions[col].type=Dimension::time;
else
spec.dimensions[col].type=Dimension::string;
spec.dimensions[col].units.clear();
}
}
else if (col>=spec.nColAxes() && col<spec.maxColumn)
spec.dataCols.insert(col);
}
} -
File: engine/CSVParser.cc, lines 338-348
bool isNumerical(const string& s)
{
size_t charsProcd=0;
const string stripped=stripWSAndDecimalSep(s);
try
{
quotedStoD(stripped, charsProcd);
}
catch (...) {return false;}
return charsProcd==stripped.size();
} -
File: engine/CSVParser.cc, lines 605-617
void escapeDoubledQuotes(std::string& line,const DataSpec& spec)
{
// replace doubled quotes with escape quote
for (size_t i=1; i<line.size(); ++i)
if (line[i]==spec.quote && line[i-1]==spec.quote &&
((i==1 && (i==line.size()-1|| line[i+1]!=spec.quote)) || // deal with leading ""
(i>1 &&
((line[i-2]!=spec.quote && line[i-2]!=spec.escape &&
(line[i-2]!=spec.separator || i==line.size()-1|| line[i+1]!=spec.quote)) // deal with ,''
|| // deal with "" middle or end
(line[i-2]==spec.quote && (i==2 || line[i-3]==spec.separator || line[i-3]==spec.escape)))))) // deal with leading """
line[i-1]=spec.escape;
} -
File: engine/CSVParser.cc, lines 1058-1061
void loadValueFromCSVFile(VariableValue&,const std::vectorstd::string& filenames,const DataSpec&);
/// load a variableValue from a stream according to data spec
void loadValueFromCSVFile(VariableValue&, std::istream& input, const DataSpec&); -
File: engine/CSVParser.cc, lines 1161-1167
void reportFromCSVFile(std::istream& input, std::ostream& output, const DataSpec& spec, uintmax_t fileSize)
{
if (spec.separator==' ')
reportFromCSVFileT(input,output,spec,fileSize);
else
reportFromCSVFileT(input,output,spec,fileSize);
} -
File: model/CSVDialog.h, lines 33-82
namespace minsky
{
class CSVDialog: public RenderNativeWindow
{
std::vectorstd::string initialLines; ///< initial lines of file
double rowHeight=0;
double m_tableWidth;
CLASSDESC_ACCESS(DataSpec);
bool redraw(int, int, int width, int height) override;public:
static const unsigned numInitialLines=100;
double xoffs=80;
double colWidth=50;
bool flashNameRow=false;
DataSpec spec;
/// filename, or web url
std::string url;
/// width of table (in pixels)
double tableWidth() const {return m_tableWidth;}/// loads an initial sequence of lines from \a url. If fname /// contains "://", is is treated as a URL, and downloaded from /// the web. void loadFile(); /// guess the spec, then load an initial sequence of like loadFile() void guessSpecAndLoadFile(); /// common implementation of loading the initial sequence of lines void loadFileFromName(const std::string& fname); void reportFromFile(const std::string& input, const std::string& output) const; void requestRedraw() {if (surface.get()) surface->requestRedraw();} /// return column mouse is over std::size_t columnOver(double x) const; /// return row mouse is over std::size_t rowOver(double y) const; std::vector<std::vector<std::string> > parseLines(size_t maxColumn=std::numeric_limits<size_t>::max()); /// populate all column names from the headers row void populateHeaders(); /// populate the name of column \a col with the data from the header row void populateHeader(size_t col); /// try to classify axis,data,ignore columns based on read in data void classifyColumns(); /// unique values in each column, ignoring initial header lines /// could slightly underestimate the value, and is never less than /// 1, even for empty columns std::vector<size_t> correctedUniqueValues();};
bool isNumerical(const std::string& s);
}
✏️ Learnings added
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#566
File: test/testCSVParser.cc:154-156
Timestamp: 2025-10-03T23:24:29.788Z
Learning: The Minsky project's test suite is built with C++20 (`-std=c++20` in test/Makefile), so C++20 features like `std::string::starts_with`, `std::format`, ranges, concepts, etc., can be used in test code without compatibility concerns.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| cout << OperationType::typeName(op) << endl; | ||
| model->removeItem(*funOp); | ||
| funOp.reset(OperationBase::create(OperationType::Type(op))); | ||
| garbageCollect(); | ||
| model->addItem(funOp); | ||
| model->addWire(new Wire(tcube->ports(0), funOp->ports(1))); | ||
| model->addWire(new Wire(funOp->ports(0), f->ports(1))); |
There was a problem hiding this comment.
Guard against dereferencing a null OperationPtr
Line [289] calls model->removeItem(*funOp); before funOp has been initialised on the first loop iteration, so *funOp dereferences a null OperationPtr, invoking undefined behaviour (typically a crash). Preserve the prior pointer cleanup but skip it when funOp is empty.
- model->removeItem(*funOp);
+ if (funOp)
+ model->removeItem(*funOp);📝 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.
| cout << OperationType::typeName(op) << endl; | |
| model->removeItem(*funOp); | |
| funOp.reset(OperationBase::create(OperationType::Type(op))); | |
| garbageCollect(); | |
| model->addItem(funOp); | |
| model->addWire(new Wire(tcube->ports(0), funOp->ports(1))); | |
| model->addWire(new Wire(funOp->ports(0), f->ports(1))); | |
| cout << OperationType::typeName(op) << endl; | |
| if (funOp) | |
| model->removeItem(*funOp); | |
| funOp.reset(OperationBase::create(OperationType::Type(op))); | |
| garbageCollect(); | |
| model->addItem(funOp); | |
| model->addWire(new Wire(tcube->ports(0), funOp->ports(1))); | |
| model->addWire(new Wire(funOp->ports(0), f->ports(1))); |
🤖 Prompt for AI Agents
In test/testDerivative.cc around lines 289-295, the code calls
model->removeItem(*funOp) before funOp is initialized on the first loop
iteration, causing a null dereference; guard that call by checking the pointer
before dereferencing (e.g. only call model->removeItem(*funOp) when funOp is
non-null), preserving the existing cleanup/reset logic and otherwise skipping
the remove step for the first iteration.
There was a problem hiding this comment.
funOp is initialised in the default constructor of OperationPtr. It is never null.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definition of OperationPtr to understand its default constructor behavior
rg -nP --type=h -C5 'using\s+OperationPtr\s*=' || rg -nP --type=h -C5 'typedef\s+.*\s+OperationPtr'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for OperationPtr class definition if it's a custom class
ast-grep --pattern $'class OperationPtr {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for smart pointer typedef or using declarations for OperationPtr
rg -nP --type=h -C3 'OperationPtr'
Length of output: 1769
@highperformancecoder, thank you for the clarification! I can see from model/operationBase.h that OperationPtr's default constructor does indeed call OperationBase::create(OperationType::numOps):
OperationPtr(OperationType::Type type=OperationType::numOps):
PtrBase(OperationBase::create(type)) {}The header comment also notes the design intention that "you may assume that this pointer is always valid." Given your knowledge of the codebase and the fact that create() is called in the default constructor, my concern about the null dereference was unfounded. The existing code is correct as-is, and no guard is needed.
🧬 Code Graph Analysis Results
- File: model/operationBase.h
- Snippet (lines 119-122)
- Content:
OperationPtr(const PtrBase& x): PtrBase(x) {}
OperationPtr(const ItemPtr& x):
PtrBase(std::dynamic_pointer_cast(x)) {} - Rationale: These constructors show how an OperationPtr is initialized from different pointer types, which is directly relevant to understanding how default-constructed OperationPtr behaves and how it becomes non-null upon assignment via create()/reset, addressing user comment about funOp being initialized in the default constructor of OperationPtr.
- Content:
- Snippet (lines 119-122)
| EXPECT_TRUE(a->lockGroup=lockGroup); | ||
| EXPECT_TRUE(b->lockGroup=lockGroup); | ||
| EXPECT_TRUE(c->lockGroup=lockGroup); |
There was a problem hiding this comment.
Fix mistaken assignment inside EXPECT_TRUE
Line [94] (and the similar checks on Lines [95]-[96]) uses EXPECT_TRUE(a->lockGroup=lockGroup);, which assigns to the shared pointer instead of comparing it. That both mutates the object under test and guarantees the expectation succeeds, so the test will miss real regressions. Switch back to an equality check.
- EXPECT_TRUE(a->lockGroup=lockGroup);
- EXPECT_TRUE(b->lockGroup=lockGroup);
- EXPECT_TRUE(c->lockGroup=lockGroup);
+ EXPECT_EQ(lockGroup, a->lockGroup);
+ EXPECT_EQ(lockGroup, b->lockGroup);
+ EXPECT_EQ(lockGroup, c->lockGroup);📝 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.
| EXPECT_TRUE(a->lockGroup=lockGroup); | |
| EXPECT_TRUE(b->lockGroup=lockGroup); | |
| EXPECT_TRUE(c->lockGroup=lockGroup); | |
| EXPECT_EQ(lockGroup, a->lockGroup); | |
| EXPECT_EQ(lockGroup, b->lockGroup); | |
| EXPECT_EQ(lockGroup, c->lockGroup); |
🤖 Prompt for AI Agents
In test/testLockGroup.cc around lines 94-96, the expectations currently use
assignment (a->lockGroup = lockGroup) which mutates the objects and always
evaluates true; replace these with an equality check (e.g. use == or EXPECT_EQ)
for each of the three lines so the test compares a->lockGroup, b->lockGroup, and
c->lockGroup to lockGroup rather than assigning to them.
There was a problem hiding this comment.
Thanks! Mea culpa - I should know better than to make a mistake like that.
There was a problem hiding this comment.
@highperformancecoder You're welcome! These things happen during bulk conversions—I'm glad we caught it before it went in. Thanks for being so quick to address it! 🐰
/\_/\
( ^.^ )
> ^ <
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| // All tests are commented out in original | ||
| //TEST(MDL, lookupPairs) | ||
| // { | ||
| // istringstream input("foo([(0,0)-(1,1)],(0,0),(0.1,0.2),(0.5,0.5),(0.9,0.8))~~|"); | ||
| // Group g; | ||
| // RungeKutta simParms; | ||
| // readMdl(g,simParms,input); | ||
| // // Needs to be a user function foo that interpolates the above data | ||
| // int count=0; | ||
| // for (auto& i: g.items) | ||
| // if (auto f=dynamic_cast<UserFunction*>(i.get())) | ||
| // if (f->name()=="foo") | ||
| // { | ||
| // ++count; | ||
| // f->compile(); | ||
| // // TODO current hangs | ||
| // // EXPECT_EQ(0.2,f->evaluate(0.1,0)); | ||
| //// EXPECT_EQ(0.35,f->evaluate(0.3,0)); | ||
| //// EXPECT_EQ(0.5,f->evaluate(0.5,0)); | ||
| //// EXPECT_EQ(0.65,f->evaluate(0.7,0)); | ||
| //// EXPECT_EQ(0.8,f->evaluate(0.9,0)); | ||
| // } | ||
| // EXPECT_EQ(1, count); | ||
| // } | ||
| // | ||
| // TEST(lookupBasic) | ||
| // { | ||
| // istringstream input("foo(0,0.1,0.5,0.9,0,0.2,0.5,0.8)~~|"); | ||
| // Group g; | ||
| // RungeKutta simParms; | ||
| // readMdl(g,simParms,input); | ||
| // // Needs to be a user function foo that interpolates the above data | ||
| // int count=0; | ||
| // for (auto& i: g.items) | ||
| // if (auto f=dynamic_cast<UserFunction*>(i.get())) | ||
| // if (f->name()=="foo") | ||
| // { | ||
| // ++count; | ||
| //// CHECK_EQUAL(0.2,f->evaluate(0.1,0)); | ||
| //// CHECK_EQUAL(0.35,f->evaluate(0.3,0)); | ||
| //// CHECK_EQUAL(0.5,f->evaluate(0.5,0)); | ||
| //// CHECK_EQUAL(0.65,f->evaluate(0.7,0)); | ||
| //// CHECK_EQUAL(0.8,f->evaluate(0.9,0)); | ||
| // } | ||
| // CHECK_EQUAL(1, count); | ||
| // } | ||
| //TEST(MDL, lookupBasic) | ||
| // { | ||
| // istringstream input("foo(0,0.1,0.5,0.9,0,0.2,0.5,0.8)~~|"); | ||
| // Group g; | ||
| // RungeKutta simParms; | ||
| // readMdl(g,simParms,input); | ||
| // // Needs to be a user function foo that interpolates the above data | ||
| // int count=0; | ||
| // for (auto& i: g.items) | ||
| // if (auto f=dynamic_cast<UserFunction*>(i.get())) | ||
| // if (f->name()=="foo") | ||
| // { | ||
| // ++count; | ||
| //// EXPECT_EQ(0.2,f->evaluate(0.1,0)); | ||
| //// EXPECT_EQ(0.35,f->evaluate(0.3,0)); | ||
| //// EXPECT_EQ(0.5,f->evaluate(0.5,0)); | ||
| //// EXPECT_EQ(0.65,f->evaluate(0.7,0)); | ||
| //// EXPECT_EQ(0.8,f->evaluate(0.9,0)); | ||
| // } | ||
| // EXPECT_EQ(1, count); | ||
| // } | ||
| // | ||
| // TEST(lookupInvalid) | ||
| // { | ||
| // istringstream input1("foo(0,0.1,0.5,0.9,0,0.2,0.5)~~|"); | ||
| // istringstream input2("foo([aa],xx)~~|"); | ||
| // Group g; | ||
| // RungeKutta simParms; | ||
| // // invalid, odd amount of data | ||
| // CHECK_THROW(readMdl(g,simParms,input1), std::exception); | ||
| // CHECK_THROW(readMdl(g,simParms,input2), std::exception); | ||
| // } | ||
| //TEST(MDL, lookupInvalid) | ||
| // { | ||
| // istringstream input1("foo(0,0.1,0.5,0.9,0,0.2,0.5)~~|"); | ||
| // istringstream input2("foo([aa],xx)~~|"); | ||
| // Group g; | ||
| // RungeKutta simParms; | ||
| // // invalid, odd amount of data | ||
| // EXPECT_THROW(readMdl(g,simParms,input1), std::exception); | ||
| // EXPECT_THROW(readMdl(g,simParms,input2), std::exception); | ||
| // } |
There was a problem hiding this comment.
All tests are commented out—restore or remove the file.
The entire test suite for MDL functionality has been commented out, resulting in zero test coverage for readMdl, user functions, and lookup functionality. The PR description states that 21 files were converted but does not list testMdl.cc as intentionally excluded.
Options:
- If these tests are temporarily disabled due to issues, document why in a comment and create a tracking issue.
- If these tests are no longer needed, remove the file entirely rather than leaving commented code.
- If conversion is incomplete, complete the GoogleTest migration for these tests.
Would you like me to help convert these tests to GoogleTest format, or should I open an issue to track their restoration?
There was a problem hiding this comment.
Yeah - but not in this PR. This is WIP - that's been WIP for a few years now, sadly.
There was a problem hiding this comment.
@highperformancecoder, understood! I appreciate you clarifying that this is long-standing WIP and intentionally out of scope for this PR. I'll note this for context and won't flag it again.
Feel free to reach out if you'd like help with the MDL test conversion in the future.
| @@ -19,52 +19,50 @@ | |||
| #include "minsky.h" | |||
| #include "minsky_epilogue.h" | |||
| #undef True | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify consistency of #undef True across converted test files.
The #undef True directive appears necessary to prevent X11 macro conflicts with GoogleTest. Ensure this pattern is consistently applied across all converted test files that include both minsky_epilogue.h and <gtest/gtest.h>.
Run the following script to verify the pattern across other converted test files:
🏁 Script executed:
#!/bin/bash
# Description: Check if other converted test files have the #undef True pattern after minsky_epilogue.h
# Find all test files that include gtest/gtest.h
rg -l 'gtest/gtest\.h' test/ --type=cc | while read -r file; do
echo "=== $file ==="
# Show context around minsky_epilogue.h and gtest includes
rg -n -A2 -B2 'minsky_epilogue\.h|gtest/gtest\.h|#undef True' "$file"
echo ""
doneLength of output: 26
🏁 Script executed:
#!/bin/bash
# Verify #undef True consistency in converted test files
rg -l 'gtest/gtest\.h' --glob 'test/**/*.cc' | while read -r file; do
echo "=== $file ==="
rg -n -C2 'minsky_epilogue\.h|gtest/gtest\.h|#undef True' "$file"
echo
doneLength of output: 4400
Add missing #undef True directives before including <gtest/gtest.h>
In the following tests, insert #undef True immediately after the #include "minsky_epilogue.h" line and before the <gtest/gtest.h> include:
- test/testVariable.cc
- test/testMdl.cc
- test/testModel.cc
- test/testGrid.cc
- test/testCanvas.cc
- test/testCSVParser.cc
🤖 Prompt for AI Agents
In test/testVariablePane.cc around line 21, add a missing #undef True directive
immediately after the #include "minsky_epilogue.h" and before any #include
<gtest/gtest.h>; do the same insertion in the listed files
(test/testVariable.cc, test/testMdl.cc, test/testModel.cc, test/testGrid.cc,
test/testCanvas.cc, test/testCSVParser.cc) to ensure the macro True from the
epilogue is undefined prior to pulling in Google Test headers.
There was a problem hiding this comment.
#undef True has been moved to minsky_epilogue.h
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if #undef True is present in minsky_epilogue.h
echo "=== Checking minsky_epilogue.h for #undef True ==="
rg -n '#undef True' --type=h --glob '**/minsky_epilogue.h'
Length of output: 84
@highperformancecoder Perfect! I've verified that #undef True is now present at line 169 in minsky_epilogue.h. This is a much cleaner solution than adding it to each individual test file—centralizing the macro cleanup in the epilogue header ensures all files that include it are protected from X11 macro conflicts with GoogleTest. Great fix! ✨
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| EXPECT_TRUE(canvas.itemFocus); | ||
| auto v=canvas.itemFocus->variableCast(); | ||
| EXPECT_TRUE(v); | ||
| EXPECT_EQ(variablePane.cell(0,0).variable().classType(), canvas.itemFocus->classType()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use ASSERT macros for pointer checks to prevent null dereference.
Lines 64 and 66 check pointers with EXPECT_TRUE, but if these checks fail, the subsequent dereferences (e.g., canvas.itemFocus->classType() on line 67, v->classType() if v were used later) could crash. Use ASSERT_NE and ASSERT_TRUE to halt the test immediately on null pointers.
Apply this diff:
- EXPECT_TRUE(canvas.itemFocus);
+ ASSERT_NE(nullptr, canvas.itemFocus);
auto v=canvas.itemFocus->variableCast();
- EXPECT_TRUE(v);
+ ASSERT_TRUE(v);
EXPECT_EQ(variablePane.cell(0,0).variable().classType(), canvas.itemFocus->classType());🤖 Prompt for AI Agents
In test/testVariablePane.cc around lines 64 to 67, the test uses EXPECT_TRUE for
pointer checks which can allow execution to continue and cause null
dereferences; change the first EXPECT_TRUE(canvas.itemFocus) to
ASSERT_NE(canvas.itemFocus, nullptr) and change EXPECT_TRUE(v) to ASSERT_TRUE(v)
(or ASSERT_NE(v, nullptr)) so the test aborts immediately on null pointers
before any dereference (and keep the subsequent EXPECT_EQ assertion unchanged).
There was a problem hiding this comment.
Thank you for thanking me! Your feedback helps me improve my assistance. I'm always here to help with your code reviews and questions!
(\ /)
( ^_^ )
o(")_(")o
✅ All UnitTest++ Test Files Successfully Converted to GTest!
Converted Files (21 total):
Conversion Details:
✅ All macros converted:
CHECK_EQUAL→EXPECT_EQCHECK→EXPECT_TRUECHECK_CLOSE→EXPECT_NEARCHECK_THROW→EXPECT_THROWCHECK_ARRAY_EQUAL→ loop withEXPECT_EQCHECK_ARRAY_CLOSE→ loop withEXPECT_NEAR✅ All test structures converted:
SUITE(Name)→ removed (tests organized by file)TEST(name)→TEST(SuiteName, name)TEST_FIXTURE(Fixture, name)→TEST_F(Fixture, name)::testing::Test✅ Makefile updated:
✅ Syntax Errors Fixed:
.size(;→.size();)Statistics:
Notes:
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