[test] Store mock state in memory, not filesystem#253
Conversation
Summary of ChangesHello @joshlf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the testing infrastructure by transitioning the mock server's state management from filesystem-based persistence to an in-memory model. This change aims to improve the performance and reliability of tests by reducing disk I/O and simplifying state synchronization. The Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the test utility's mock server by moving from file-based state management to an in-memory, Arc<RwLock> protected MockState accessed via an HTTP server. The mock git binary now acts as a client, forwarding commands to this new mock server using ureq, which then handles the logic, updates the shared state, and decides whether to passthrough to the real git. This change involved adding ureq and related dependencies, and updating test contexts to interact with the in-memory state. A review comment highlighted a potential concurrency issue in the new handle_git mock server endpoint, suggesting that the write lock on the MockState should only be acquired when git push commands are processed, rather than at the beginning of every git command, to improve parallel test execution.
c231793 to
a3a5a75
Compare
e4b76f8 to
4fb566e
Compare
a3a5a75 to
6f12e4d
Compare
4fb566e to
c3fe941
Compare
6f12e4d to
a6d09ac
Compare
c3fe941 to
1bef753
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on refactoring the test infrastructure to use an in-memory mock server state managed by Arc<RwLock<MockState>> instead of file-based mock_state.json for improved test reliability and performance. This change involved updating testutil/src/lib.rs to manage the mock state directly and modifying test files like tests/prevent_push_to_closed_pr.rs and tests/reproduce_pagination_bug.rs to interact with this new in-memory state. The mock git binary (testutil/src/mock_bin.rs) was refactored to communicate with the mock HTTP server via ureq for Git command interception and response handling, replacing its previous file-locking mechanism. Correspondingly, testutil/src/mock_server.rs was updated to include a new handle_git endpoint for processing these requests and managing the shared MockState. Additionally, the Cargo.lock file received numerous dependency updates across various crates, and testutil/Cargo.toml removed the gix dependency while adding ureq. A [[bin]] entry for gherrit was also added to Cargo.toml. A review comment suggested restoring more descriptive expect messages in testutil/src/mock_server.rs for better debugging, specifically for serde_json::from_str calls related to parsing title and body fields.
a6d09ac to
82fb137
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the test suite's mocking infrastructure to store mock state in memory instead of on the filesystem. This is a significant improvement that should lead to more reliable and faster tests by avoiding filesystem I/O and potential race conditions.
The core changes involve:
- Using an
Arc<RwLock<MockState>>to manage shared, mutable state in memory for the mock server. - Reworking the mock
gitbinary to act as an HTTP proxy to the mock server, centralizing mocking logic. - Updating test helpers and tests to interact with the new in-memory state management.
The changes are well-implemented and follow the stated goal. I've only left a few minor suggestions in testutil/src/mock_server.rs to improve the debuggability of tests by using expect() with descriptive messages instead of unwrap().
82fb137 to
183f60e
Compare
59c1e16 to
4531e02
Compare
c9eaad5 to
7522e34
Compare
gherrit-pr-id: G86ee0f76d74006e946270bbc7c09349115e82fe1
7522e34 to
e44cf98
Compare
Latest Update: v15 — Compare vs v14
📚 Full Patch History
Links show the diff between the row version and the column version.