Used Antigravity to add some unit tests. #601
Conversation
|
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. 📝 WalkthroughWalkthroughExports build flags, switches lcov coverage flow to a gcovr-based target, guards against empty-string access in MDL parsing, defaulted DataOp copy-assignment to return non-const reference, updates gui-js devDependencies to NX, expands test build objects/flags, and adds GoogleTest suites for DataOp, DatabaseIngestor, Lock, and MdlReader. Changes
Sequence Diagram(s)(Skipped — changes are build/test additions and a parsing guard; no new multi-component runtime control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 8
🤖 Fix all issues with AI agents
In `@engine/mdlReader.cc`:
- Around line 295-298: The code accesses definition[0] in the switch without
ensuring definition is non-empty; update the logic around
readToken/collapseWS/trimWS so that after assigning definition =
collapseWS(trimWS(readToken(mdlFile,'~'))), you check if definition.empty() and
handle that case (e.g., set a safe default char or skip the switch) before using
definition[0]; modify the switch usage in the same block (where nameStr and
definition are used) to rely only on a guarded/non-empty definition.
In `@gui-js/package.json`:
- Around line 150-152: Replace the deprecated dependency "@nrwl/workspace":
"^19.8.4" with "@nx/workspace": "21.x" and align the other Nx packages
("@nx/jest", "@nx/js") to version 21 (e.g., "21.2.1") to avoid namespace/version
skew; after updating the package.json entries for "@nrwl/workspace" →
"@nx/workspace" and bumping "@nx/*" deps to 21, run the migration command `nx
migrate 21` and then `pnpm/npm/yarn install` and `nx migrate --run-migrations`
to apply automatic migrations.
In `@Makefile`:
- Around line 557-561: The Makefile defines an lcov target but doesn't declare
it as phony, so if a file named "lcov" exists Make will skip the recipe; add a
.PHONY declaration that includes the lcov target (e.g., add ".PHONY: lcov"
alongside other phony targets) so the lcov recipe (which references LCOV_FLAGS
and runs "$(MAKE) clean") always executes regardless of filesystem state.
In `@test/testDataOp.cc`:
- Around line 20-26: TearDown() in test/testDataOp.cc calls
std::filesystem::exists and std::filesystem::remove but the <filesystem> header
is missing; add `#include` <filesystem> to the include block (near the existing
includes such as "dataOp.h" and <fstream>) so the compiler can resolve
std::filesystem symbols used in TearDown().
- Around line 111-118: The current test InterpolateOutsideRange uses no-op
assertions (EXPECT_TRUE(std::isfinite(val) || true)), which always pass; update
the assertions for val and val2 in test/testDataOp.cc (the variables produced by
op.interpolate(0.5) and op.interpolate(2.5)) to assert real expected semantics:
either require finiteness (replace with EXPECT_TRUE(std::isfinite(val)) and
EXPECT_TRUE(std::isfinite(val2))) or, if the intended behavior is
clamping/extrapolation, assert the concrete values against the endpoints
(compare to op.interpolate(0.0) or op.interpolate(last_index) or the known
endpoint values using EXPECT_DOUBLE_EQ/EXPECT_NEAR). Remove the "|| true"
placeholder and choose one of those concrete checks to make the test meaningful.
In `@test/testLock.cc`:
- Around line 103-109: The test LockTest::Units calls Lock::units() and obtains
a Units value but contains no assertion; update the test to verify behavior by
either wrapping the call in EXPECT_NO_THROW(Lock::units()) if only absence of
exceptions is intended, or add an explicit EXPECT/ASSERT on the returned Units
(e.g., compare against a known expected Units value or a dimensionless sentinel)
using the local variables Lock lock, Units u and the units() method to locate
the change.
- Around line 145-153: Fix the typo in the test comment ("Toasted" → "Test") and
eliminate the unused-variable warnings by either removing the local references
or using them; specifically, in TEST_F(LockTest, StaticIcons) update the comment
and either drop the auto& locked/unlocked declarations that reference
Lock::lockedIcon and Lock::unlockedIcon, or keep them but add a benign use
(e.g., an assertion or a (void)locked/(void)unlocked cast) so the compiler sees
them as used.
In `@test/testMdlReader.cc`:
- Around line 168-176: The test loop over group->items may never find a
VariableBase and will silently pass; declare a bool flag (e.g., foundVariable =
false) before the for-loop, set foundVariable = true when you successfully
locate a VariableBase with a non-null vValue() (the same place you currently
break), then after the loop add an assertion (e.g., EXPECT_TRUE or ASSERT_TRUE
on foundVariable) to fail the test if no variable was found; reference the
existing symbols group->items, VariableBase, vValue(), and
vv->sliderMax/sliderMin to locate where to add the flag and assertion.
🧹 Nitpick comments (6)
gui-js/package.json (1)
185-185:nxis typically a devDependency, not a runtime dependency.The
nxbuild tool is placed independencies, but it's a build/development tool that isn't needed at runtime. Consider moving it todevDependenciesunless there's a specific reason for it to be a runtime dependency.♻️ Proposed fix
Move
nxfromdependenciestodevDependencies:"devDependencies": { ... + "nx": "^21.2.1", ... }, "dependencies": { ... - "nx": "^21.2.1", ... }test/testLock.cc (1)
48-55: Static analysis: consider usingconstfor unmodified copy.The static analysis tool flags that
lock2is never modified after being copied fromlock1. While this is intentional for testing copy construction, you can silence the warning by makinglock2const, which also documents the test's intent more clearly.♻️ Proposed fix
TEST_F(LockTest, CopyConstructor) { Lock lock1; - Lock lock2(lock1); + const Lock lock2(lock1); // Copy should also be unlocked initially EXPECT_FALSE(lock2.locked()); }test/testDatabaseIngestor.cc (3)
43-49:createTestCSVdoesn't verify file creation success.The helper creates a file but doesn't check if the file was successfully opened or written. If file creation fails (e.g., due to permissions), subsequent tests may fail with unclear errors.
♻️ Proposed fix
string createTestCSV(const string& filename, const string& content) { ofstream file(filename); + if (!file.is_open()) { + throw std::runtime_error("Failed to create test file: " + filename); + } file << content; file.close(); return filename; }
63-71: Test has no meaningful assertion.The
Constructiontest accessesdi.dbanddi.specbut doesn't validate any expected behavior. TheEXPECT_NO_THROWonly confirms access doesn't throw, which is weak coverage. Consider asserting on expected default values.
99-106: Test has no assertion after configuring spec.The
SpecConfigurationtest setsingestor.spec.separatorbut doesn't verify it was actually set. Consider adding an assertion to confirm the value.♻️ Proposed fix
TEST_F(DatabaseIngestorTest, SpecConfiguration) { // Test that the spec member can be configured - EXPECT_NO_THROW({ - ingestor.spec.separator = ','; - // spec can be configured before import - }); + ingestor.spec.separator = ','; + EXPECT_EQ(ingestor.spec.separator, ','); }test/testDataOp.cc (1)
178-205:EmptyDataandUnitstests don't assert any behavior.Both tests will pass even if behavior regresses. Consider adding explicit expectations (return value, exception, or units invariants), or remove until behavior is defined.
There was a problem hiding this comment.
Pull request overview
This PR adds new GoogleTest-based unit tests around MDL parsing, Lock behavior, database ingestion, and DataOp functionality, alongside a small MDL reader robustness tweak and build/dependency updates.
Changes:
- Added new C++ unit test files for
mdlReader,Lock,DatabaseIngestor, andDataOp, and wired them into the test build. - Improved robustness in
engine/mdlReader.ccwhen handling empty tokens. - Updated build/config files (Makefiles) and adjusted
gui-jsdependencies/lockfile.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testMdlReader.cc | Adds unit tests for MDL parsing scenarios (control block, variables, INTEG, lookup, comments, slider spec). |
| test/testLock.cc | Adds unit tests for Lock construction and behaviors (toggle, draw, ports). |
| test/testDatabaseIngestor.cc | Adds basic DatabaseIngestor tests (with one import test disabled). |
| test/testDataOp.cc | Adds unit tests for DataOp read/interpolate/derivative/serialization behaviors. |
| test/Makefile | Includes new test objects and expands include paths for test builds. |
| model/dataOp.h | Updates DataOp assignment operator to a defaulted DataOp& operator=.... |
| gui-js/package.json | Formatting changes and adds nx dependency. |
| gui-js/package-lock.json | Lockfile updates reflecting dependency graph changes (incl. nx). |
| engine/mdlReader.cc | Adds guard against calling .back() on an empty token. |
| Makefile | Exports build variables and changes lcov target behavior/output generation. |
Files not reviewed (1)
- gui-js/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
engine/mdlReader.cc:299
definitioncan be empty here (eg if the token didn’t end with '=' or the file ends unexpectedly), but the code unconditionally indexesdefinition[0], which is undefined behavior and can crash. Guard theswitchwithif (!definition.empty())(or throw a parse error when a definition is required).
if (!nameStr.empty() && nameStr.back()=='=')
// only read definition if this was a variable definition
definition=collapseWS(trimWS(readToken(mdlFile,'~')));
switch (definition[0])
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 557-573: The gcovr invocation in the lcov Makefile target is using
a positional output path instead of the required -o flag; update the gcovr
command in the lcov target (the command under the lcov recipe that currently
reads `gcovr -r . --html --html-details coverage/index.html`) to include the -o
option so it becomes `gcovr -r . --html --html-details -o coverage/index.html`,
ensuring the coverage HTML is written to coverage/index.html when running the
lcov target.
In `@test/testDataOp.cc`:
- Around line 67-84: The test currently asserts the wrong size: in TEST_F
DataOpTest::InitRandom the call to op.initRandom(xmin, xmax, numSamples)
produces numSamples points (because the implementation loops with x < xmax and
dx = (xmax-xmin)/numSamples), so change the assertion EXPECT_EQ(numSamples + 1,
op.data.size()) to EXPECT_EQ(numSamples, op.data.size()) and keep the subsequent
range checks for elements in op.data.
🧹 Nitpick comments (5)
test/testDatabaseIngestor.cc (2)
32-60: Consider verifying file creation success increateTestCSV.The helper function doesn't check if the file was successfully opened before writing. If the file cannot be created (e.g., permissions issue), the test will silently proceed with an invalid/empty file.
Proposed fix
string createTestCSV(const string& filename, const string& content) { ofstream file(filename); + if (!file.is_open()) + throw std::runtime_error("Failed to create test file: " + filename); file << content; file.close(); return filename; }
63-71: Test assertions have no meaningful validation.The
Constructiontest accessesdi.dbanddi.specbut doesn't validate any properties. The current assertions only verify that accessing these members doesn't throw, which provides minimal coverage.Proposed improvement
TEST_F(DatabaseIngestorTest, Construction) { // Test that DatabaseIngestor can be constructed DatabaseIngestor di; - EXPECT_NO_THROW({ - auto& db = di.db; - auto& spec = di.spec; - }); + // Verify members are accessible and have sensible defaults + EXPECT_NO_THROW((void)di.db); + EXPECT_NO_THROW((void)di.spec); }test/testDataOp.cc (2)
191-198: Units test has no assertion.The test retrieves units but doesn't validate anything. Consider wrapping in
EXPECT_NO_THROWor asserting expected units.Proposed fix
TEST_F(DataOpTest, Units) { DataOp op; - // Test units passthrough from output port - Units u = op.units(false); - // Should return units from connected port or dimensionless + // Test that units can be queried without throwing + EXPECT_NO_THROW({ + Units u = op.units(false); + (void)u; // suppress unused variable warning + }); }
200-214: PackUnpack test doesn't verify data content after round-trip.The test checks that sizes match after unpack, but doesn't verify that the actual data values were correctly serialized and deserialized.
Proposed improvement
EXPECT_EQ(op1.data.size(), op2.data.size()); + // Verify actual data values survived the round-trip + for (const auto& [key, value] : op1.data) { + ASSERT_TRUE(op2.data.contains(key)); + EXPECT_EQ(value, op2.data.at(key)); + } }test/testLock.cc (1)
40-55: Consider usingconstfor unmodified copy inCopyConstructortest.Per static analysis,
lock2is never modified after being copy-constructed. Usingconstclarifies intent and silences the warning.Proposed fix
TEST_F(LockTest, CopyConstructor) { Lock lock1; - Lock lock2(lock1); + const Lock lock2(lock1); // Copy should also be unlocked initially EXPECT_FALSE(lock2.locked()); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/testMdlReader.cc`:
- Around line 33-40: The test fixture holds persistent state in members GroupPtr
group and Simulation simParms which are initialized once and can accumulate
across tests; modify MdlReaderTest::SetUp() to reinitialize these per test by
assigning group = std::make_shared<Group>() and resetting group->self = group,
and also reset simParms to a clean default (e.g., assign a fresh Simulation or
call its clear/reset method) so each test starts with a fresh Group and
Simulation instance.
This change is
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.