docs: update dev docs for running tests - #3157
Conversation
WalkthroughOhayo, sensei! This update refines the instructions in the DEVELOPMENT.md file. The installation step has been reworded from "Install Rust and run the test suite to confirm your setup:" to "Install & update Rust:". Additionally, the documentation now specifies that users should generate database artifacts via Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ArtifactGenerator as Artifact Script
participant TestRunner as Nextest
User->>ArtifactGenerator: Run 'bash scripts/rebuild_test_artifacts.sh'
ArtifactGenerator-->>User: Database artifacts generated
User->>TestRunner: Run 'cargo test'
TestRunner-->>User: Returns test results
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
DEVELOPMENT.md (3)
21-22: Ohayo, sensei – Clarify the Rust update step.
The new instruction “Install & update Rust:” now runsrustup override set stable && rustup update && cargo testin one go. This may lead to tests running before the required database artifacts are generated in later steps. Please consider clarifying whether running tests here is intentional or if the test step should be separated until after the artifacts are ready.
27-35: Ohayo, sensei – Review the test execution sequence.
The updated section instructs users to first generate DB artifacts (bash scripts/rebuild_test_artifacts.sh) and then run tests (cargo test), which is a great enhancement. However, note that the previous Rust update step also runscargo test. This might cause duplicated test execution or confusion about the intended order of operations. Please consider consolidating these commands or adding a note to explain the rationale behind running tests in two separate blocks.
51-52: Ohayo, sensei – Nice nextest note addition.
The update clearly states thatnextestis used as the test runner and provides a helpful link. For even better guidance, you might consider adding a brief note or a link for installation/setup instructions fornextest, just in case some developers are not yet familiar with it.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
crates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockcrates/torii/types-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
DEVELOPMENT.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
2e7a037 to
3b9aeeb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3157 +/- ##
=======================================
Coverage 55.60% 55.61%
=======================================
Files 443 443
Lines 62962 62962
=======================================
+ Hits 35012 35017 +5
+ Misses 27950 27945 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3b9aeeb to
6f169c9
Compare
6f169c9 to
0265eaa
Compare
kariy
left a comment
kariy
left a comment
There was a problem hiding this comment.
Thanks @cwkang1998, can you restore back the tar files ?
There was a problem hiding this comment.
Sure, should the db tar and also the changes to the scarb lock files during test runs be generally ignored and reverted for other contributions too?
Is there any reason these test related files are tracked in version control?
There was a problem hiding this comment.
Iirc we don't really have a strong reason to actually track these files other than to make the CI run faster and have a slightly more controlled environment. Considering the changes that might break the test files happen only so frequently (eg changes on Cairo version or core Cairo lib).
Unless I'm wrong on this CC @glihm .
| # First generate db artifacts for the tests | ||
| bash scripts/rebuild_test_artifacts.sh |
There was a problem hiding this comment.
ah this should actually just be:
| # First generate db artifacts for the tests | |
| bash scripts/rebuild_test_artifacts.sh | |
| # First generate db artifacts for the tests | |
| bash scripts/extract_test_db.sh |
because we already tracked the compressed db files
2 participants
Description
Mainly updating the
DEVELOPMENT.mddocument to include more details on running the tests initially. Based on what I faced during development.Related issue
N/A
Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
nextestas the test runner, with a link for more information.