tests: Simplify smoke test setup #7225
Conversation
|
Warning Rate limit exceeded@Teebor-Choka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the smoke test setup by removing pre-existing DB dependencies and cleaning up leftover log files. Key changes include:
- Removing the DB snapshot file copying from snapshot creation and adjusting cleanup in Snapshot.reuse().
- Modifying node setup to support a dynamic log file name via a new log_tag parameter.
- Cleaning up old log files and adjusting the shared node bringup process in Main Process and Cluster modules.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sdk/python/localcluster/snapshot.py | Removed DB file copying and modified cleanup logic for non-snapshot files. |
| sdk/python/localcluster/node.py | Added a new log_tag parameter to node setup for dynamic log file naming. |
| sdk/python/localcluster/main_process.py | Added cleanup of old log files before starting smoke tests. |
| sdk/python/localcluster/cluster.py | Changed shared_bringup signature by removing skip_funding and updating setup. |
Comments suppressed due to low confidence (2)
sdk/python/localcluster/snapshot.py:45
- The logic for cleaning up non-snapshot directories seems to have been removed, as the previous implementation removed entire directories using shutil.rmtree. If this cleanup is still needed to avoid leftover directories, adjust the logic to remove non-snapshot directories explicitly.
for entry in self.parent_dir.glob("*"):
sdk/python/localcluster/cluster.py:49
- Replacing the 'skip_funding' parameter with 'log_tag' changes the behavior of shared_bringup by removing the automatic funding of nodes. If this behavior is intended, please update the corresponding documentation or API comments.
async def shared_bringup(self, log_tag: str = ""):
fc2a581 to
c110d18
Compare
c110d18 to
0a8c173
Compare
The smoke tests no longer need to address the pre-existing DB, since it has been moved into a randomly generated directory to avoid clashes.