fix(tests): use in-memory secure storage to eliminate keychain prompts#299
fix(tests): use in-memory secure storage to eliminate keychain prompts#299Super-Genius wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888eeedbe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if(BUILD_TESTING) | ||
| add_compile_definitions(SGNS_USE_MEMORY_SECURE_STORAGE) |
There was a problem hiding this comment.
Because BUILD_TESTING is set to ON by default near the top of this file, this global definition is applied to add_subdirectory(src) in a default build, not just to test executables. Any application/library code that includes SecureStorage.hpp (for example GeniusAccount::CreateSecureStorage) will therefore alias SecureStorageImpl to MemorySecureStorage, so accounts generated by a default build are kept only in process memory while the public-key index is still written to disk; after restart the key cannot be loaded from the OS store. Please restrict this define to test targets or make it opt-in only for the specific tests that need it.
Useful? React with 👍 / 👎.
Split genius_node into two libraries: - genius_node: production, uses OS keychain (Apple/Windows/Linux) - genius_node_test: tests, uses MemorySecureStorage (in-memory) All test targets link against genius_node_test. No global cmake flags. Also adds MemorySecureStorage implementation and updates secure_storage_test to use it on macOS. Addresses Codex review P1: keep memory storage out of non-test targets.
Super-Genius
force-pushed
the
fix/test-memory-secure-storage
branch
from
888eeed to
82ac885
Compare
…rompts Replace compile-time SGNS_USE_MEMORY_SECURE_STORAGE approach with a runtime DI factory in GeniusAccount. Tests call SetSecureStorageFactory() to inject MemorySecureStorage, eliminating macOS keychain prompts without relying on platform-specific compile flags. Changes: - Add SecureStorageFactory type and Set/GetSecureStorageFactory() to GeniusAccount - CreateSecureStorage() uses injected factory when set, falls back to default - Existing tests inject MemorySecureStorage via factory in SetUp/fixture - Remove stale test CMakeLists entries from other branches - Keep genius_node_test library split from previous commit All 60 tests pass with zero keychain prompts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3d1af079e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| add_library(genius_node_test ${GENIUS_NODE_SOURCES}) | ||
| target_link_libraries(genius_node_test ${GENIUS_NODE_LIBS}) |
There was a problem hiding this comment.
For test binaries that were switched to link genius_node_test but never call GeniusAccount::SetSecureStorageFactory (for example test/src/processing_nodes/processing_nodes_test.cpp:40 creates a node via GeniusNode::New), this target is compiled from the same sources and link libraries as genius_node and does not define SGNS_USE_MEMORY_SECURE_STORAGE, so CreateSecureStorage still falls back to the platform SecureStorageImpl and can hit the macOS keychain. Please either add the memory-storage compile definition to this target or install the factory in those test binaries; otherwise several tests still prompt despite linking the new target.
Useful? React with 👍 / 👎.
- 8 new auto-filed test-failure bugs (#300-#307) - 3 open PRs: #309 bridge_phase5, #308 crdt-3.5-backport, #299 memory-secure-storage - Updated bridge, consensus perf, and stale-item sections - Gate violations noted: no PR/issue templates exist in repo Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
EduMenges
force-pushed
the
develop
branch
3 times, most recently
from
128165d to
afb4967
Compare
1 participant
Summary
Replaces macOS keychain-backed secure storage with in-memory storage for test builds. Eliminates ~20 keychain password prompts per test run.
Changes
src/local_secure_storage/impl/MemorySecureStorage.hpp— in-memory JSON backend, no OS keychain accessSecureStorage.hpp— respectsSGNS_USE_MEMORY_SECURE_STORAGEcompile flagCommonBuildParameters.cmake— adds flag before library build whenBUILD_TESTING=ONsecure_storage_test.cpp— uses MemorySecureStorage on macOSHow it works
When
BUILD_TESTING=ON, the cmake build adds-DSGNS_USE_MEMORY_SECURE_STORAGEglobally. This causesSecureStorageImplto resolve toMemorySecureStorage(in-memory map) instead ofAppleSecureStorage(macOS keychain). Both library and test targets get the flag.Testing
cd build/OSX/Debug cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Debug ninja -j4 ctest --output-on-failure -j4Zero keychain prompts expected.