Add test for nix_store_build_paths#372
Conversation
📝 WalkthroughWalkthroughA new test case Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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/libstore-tests/nix_api_store.cc (1)
958-963: Consider usingnix_api_store_test_basefixture instead.This test uses the
nix_api_store_testfixture but then creates its own store viaopen_local_store(). Other tests in this file that need to configure experimental features before opening a store (e.g.,build_from_jsonat line 352,nix_store_realise_invalid_systemat line 399) usenix_api_store_test_baseinstead.Using
nix_api_store_test_basewould be more consistent with the established pattern and avoids shadowing the fixture'sstoremember.♻️ Suggested change
-TEST_F(nix_api_store_test, nix_store_build_paths) +TEST_F(nix_api_store_test_base, nix_store_build_paths)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore-tests/nix_api_store.cc` around lines 958 - 963, The test nix_store_build_paths currently uses the nix_api_store_test fixture but calls open_local_store() and shadows the fixture's store member; change the test to use the nix_api_store_test_base fixture (replace TEST_F(nix_api_store_test, ...) with TEST_F(nix_api_store_test_base, ...)) and remove the manual open_local_store() call so the test uses the fixture-provided store member, keeping the existing experimental feature setup (nix::experimentalFeatureSettings.set(...)) before the store is opened by the fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libstore-tests/nix_api_store.cc`:
- Line 979: The assertion after calling nix_add_derivation is checking the wrong
variable (ASSERT_NE(drv, nullptr)); change it to assert the derivation path
pointer returned by nix_add_derivation (ASSERT_NE(drvPath, nullptr)) so the test
verifies drvPath is non-null; update the assertion that references drv to use
drvPath in the test around the nix_add_derivation call.
---
Nitpick comments:
In `@src/libstore-tests/nix_api_store.cc`:
- Around line 958-963: The test nix_store_build_paths currently uses the
nix_api_store_test fixture but calls open_local_store() and shadows the
fixture's store member; change the test to use the nix_api_store_test_base
fixture (replace TEST_F(nix_api_store_test, ...) with
TEST_F(nix_api_store_test_base, ...)) and remove the manual open_local_store()
call so the test uses the fixture-provided store member, keeping the existing
experimental feature setup (nix::experimentalFeatureSettings.set(...)) before
the store is opened by the fixture.
|
|
||
| auto * drvPath = nix_add_derivation(ctx, store, drv); | ||
| assert_ctx_ok(); | ||
| ASSERT_NE(drv, nullptr); |
There was a problem hiding this comment.
Bug: Wrong variable checked in assertion.
After nix_add_derivation, the assertion checks drv instead of drvPath. This appears to be a copy-paste error from line 975.
🐛 Proposed fix
auto * drvPath = nix_add_derivation(ctx, store, drv);
assert_ctx_ok();
- ASSERT_NE(drv, nullptr);
+ ASSERT_NE(drvPath, nullptr);📝 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.
| ASSERT_NE(drv, nullptr); | |
| auto * drvPath = nix_add_derivation(ctx, store, drv); | |
| assert_ctx_ok(); | |
| ASSERT_NE(drvPath, nullptr); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libstore-tests/nix_api_store.cc` at line 979, The assertion after calling
nix_add_derivation is checking the wrong variable (ASSERT_NE(drv, nullptr));
change it to assert the derivation path pointer returned by nix_add_derivation
(ASSERT_NE(drvPath, nullptr)) so the test verifies drvPath is non-null; update
the assertion that references drv to use drvPath in the test around the
nix_add_derivation call.
2 participants
Motivation
Adds a test for
nix_store_build_pathsContext
In #210, we added
nix_store_build_paths. While upstreaming in NixOS#15352, a test was added to ensure this function works.Summary by CodeRabbit