Skip to content

improve the starting paragraphs of testing section - #13139

Closed
ShahakShama wants to merge 2 commits into
shahak/style-guide-improve-macrosfrom
shahak/style-guide-improve-testing-opening
Closed

ShahakShama wants to merge 2 commits into
shahak/style-guide-improve-macrosfrom
shahak/style-guide-improve-testing-opening

Conversation

@ShahakShama

@ShahakShama ShahakShama commented Mar 10, 2026 •

Copy link
Copy Markdown
Contributor

Note

Low Risk
Documentation-only edits that restructure and clarify testing guidelines; no runtime or behavior changes.

Overview
Updates style.md’s Testing section to improve readability and organization.

Clarifies assertion guidance (preferring assert_eq over is_ok, with unwrap() as an alternative) and reorganizes test recommendations into explicit subsections for Integration/Flow tests and Binary tests, including revised guidance on where to place tests and how to test binaries via a lib.rs entrypoint.

Written by Cursor Bugbot for commit 299b5b6. This will update automatically on new commits. Configure here.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

ShahakShama commented Mar 10, 2026 •

Copy link
Copy Markdown
Contributor Author

@ShahakShama
ShahakShama force-pushed the shahak/style-guide-improve-macros branch from c31bfd9 to 79461e4 Compare March 15, 2026 14:58
@ShahakShama
ShahakShama force-pushed the shahak/style-guide-improve-testing-opening branch from 6531dc0 to 3b6c285 Compare March 15, 2026 14:58
@ShahakShama
ShahakShama force-pushed the shahak/style-guide-improve-macros branch from 79461e4 to 450475f Compare March 16, 2026 07:38
@ShahakShama
ShahakShama force-pushed the shahak/style-guide-improve-testing-opening branch from 3b6c285 to 299b5b6 Compare March 16, 2026 07:38

@Stavbe Stavbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stavbe made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on ShahakShama).


style.md line 79 at r1 (raw file):

Use unit tests for short (< 1 sec) and threadsafe tests, otherwise use cargo integration tests:

Put flow tests and integration tests inside `tests/` at the crate root if they don't depend on features of their crate (this constraint is valid until [this cargo issue](https://github.com/rust-lang/cargo/issues/2911#issuecomment-1739880593) or [this cargo bug](https://github.com/rust-lang/cargo/issues/15151) get resolved), otherwise put them in a dedicated integration-test crate for the whole package (mostly relevant for multi-crate packages). Cargo integration test files are not parallelized, and are run in an anonymous crate without `cfg(test)`, which allows the test writer to simulate real UX.

can you explain?

Code quote:

 depend on features of their crate 

@ShahakShama ShahakShama left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShahakShama made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on Stavbe).


style.md line 79 at r1 (raw file):

Previously, Stavbe wrote…

can you explain?

Is this better?

@ShahakShama
ShahakShama force-pushed the shahak/style-guide-improve-testing-opening branch from 2ffe0d2 to 1e0edd5 Compare April 9, 2026 12:15

@Stavbe Stavbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stavbe made 3 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on ShahakShama).


style.md line 79 at r1 (raw file):

Previously, ShahakShama wrote…

Is this better?

yes, thanks


style.md line 77 at r2 (raw file):

### Integration and Flow Tests
Use unit tests for short (< 1 sec) and threadsafe tests, otherwise use cargo integration tests:

Maybe we should explain more about it?

Code quote:

 cargo integration tests:

style.md line 89 at r2 (raw file):

### Binary Tests

To test binary crates, add a `lib.rs` and call its main from `main.rs` and from the test.

Where to put the tests?

Code quote:

To test binary crates, add a `lib.rs` and call its main from `main.rs` and from the test.

@ShahakShama
ShahakShama force-pushed the shahak/style-guide-improve-testing-opening branch from 1e0edd5 to da7728c Compare April 13, 2026 10:51

@ShahakShama ShahakShama left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShahakShama made 2 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on Stavbe).


style.md line 77 at r2 (raw file):

Previously, Stavbe wrote…

Maybe we should explain more about it?

IMO no need


style.md line 89 at r2 (raw file):

Previously, Stavbe wrote…

Where to put the tests?

I don't think we need to specify this

@ShahakShama
ShahakShama force-pushed the shahak/style-guide-improve-testing-opening branch from da7728c to 9513439 Compare April 28, 2026 13:43
@github-actions

Copy link
Copy Markdown

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale.
This PR will be closed and locked in 7 days if no further activity occurs.
Thank you for your contributions!

@github-actions github-actions Bot added the stale label May 29, 2026
@github-actions github-actions Bot closed this Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants