Moved Cairo code snippets to Scarb projects in docs/listings
#2470
+2,185
−1,434
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
integraledelebesgue
requested review from
ksew1,
cptartur and
ddoktorski
as code owners
integraledelebesgue
changed the title
listings Scarb project
integraledelebesgue
changed the title
listings Scarb project
integraledelebesgue
force-pushed
the
spr/master/e2a3655e
branch
3 times, most recently
from
7a70cdf
to
af1a24e
Compare
integraledelebesgue
changed the title
listings Scarb projectdocs/listings
integraledelebesgue
changed the title
docs/listingslistings Scarb project
integraledelebesgue
force-pushed
the
spr/master/e2a3655e
branch
from
af1a24e
to
0536710
Compare
integraledelebesgue
changed the title
listings Scarb projectdocs/listings
integraledelebesgue
force-pushed
the
spr/master/e2a3655e
branch
7 times, most recently
from
0080314
to
4515227
Compare
cptartur
reviewed
Sep 17, 2024
Comment on lines
29
to
+33
| ```rust | ||
| #[test] | ||
| fn call_and_invoke() { | ||
| // ... | ||
|
|
||
| let balance = dispatcher.get_balance(); | ||
| assert(balance == 0, 'balance == 0'); | ||
|
|
||
| dispatcher.increase_balance(100); | ||
|
|
||
| let balance = dispatcher.get_balance(); | ||
| assert(balance == 100, 'balance == 100'); | ||
| } | ||
| {{#include ../../listings/snforge_overview/crates/using_cheatcodes/tests/caller_address/failing.cairo}} | ||
| ``` | ||
|
|
||
| However, when running this test, we will get a failure with a message | ||
| This test passes, which means that `increase_balance` method panics with expected message. |
There was a problem hiding this comment.
Why was this changed to should panic?
There was a problem hiding this comment.
We cannot have panicking tests since we want to verify Cairo listings using scarb test. That's why some tests are marked #[should_panic] and some use safe dispatchers.
Also a few tests in the first chapter introducing to testing are presented as failing but in fact they have a hidden #[should_panic] not appearing in the text.
There was a problem hiding this comment.
I've looked at the whole document again and I think this is confusing to have it pass. If that's necessary you can add hidden should_panic but we should be showing to the users that this fails so it's obvious what's the point of using a cheat there.
integraledelebesgue
force-pushed
the
spr/master/e2a3655e
branch
4 times, most recently
from
34093f4
to
0eba6f9
Compare
commit-id:e2a3655e
integraledelebesgue
force-pushed
the
spr/master/e2a3655e
branch
from
0eba6f9
to
5df9a0b
Compare
cptartur
reviewed
Sep 20, 2024
| let contract_address: ContractAddress = 0x1e52f6ebc3e594d2a6dc2a0d7d193cb50144cfdfb7fdd9519135c29b67e427 | ||
| .try_into() | ||
| .expect('Invalid contract address value'); | ||
| Structure use by the command: |
There was a problem hiding this comment.
Suggested change
| Structure use by the command: | |
| Structure used by the command: |
| @@ -1,5 +1,9 @@ | |||
| # Conditional Compilation | |||
|
|
|||
| > 📝 **Note** | |||
| > For an elaborative guide on Scarb conditional compilation, please refer to [Scarb documentation](https://docs.swmansion.com/scarb/docs/reference/conditional-compilation.html) | |||
There was a problem hiding this comment.
Suggested change
| > For an elaborative guide on Scarb conditional compilation, please refer to [Scarb documentation](https://docs.swmansion.com/scarb/docs/reference/conditional-compilation.html) | |
| > For more detailed guide on Scarb conditional compilation, please refer to [Scarb documentation](https://docs.swmansion.com/scarb/docs/reference/conditional-compilation.html) |
Comment on lines
+86
to
90
| > Cheats related to contract execution context won't work for forked contracts written in **Cairo 0**. | ||
| > Those cheatcodes are not going to have any effects: | ||
| > | ||
| > - start_spoof / stop_spoof | ||
| > - spy_events |
There was a problem hiding this comment.
Suggested change
| > Cheats related to contract execution context won't work for forked contracts written in **Cairo 0**. | |
| > Those cheatcodes are not going to have any effects: | |
| > | |
| > - start_spoof / stop_spoof | |
| > - spy_events | |
| > Cheats related to contract execution context won't work for forked contracts written in **Cairo 0**. | |
| > Those cheatcodes are not going to have any effects: | |
| > | |
| > - `start_spoof` / `stop_spoof` | |
| > - `spy_events` |
Comment on lines
29
to
+33
| ```rust | ||
| #[test] | ||
| fn call_and_invoke() { | ||
| // ... | ||
|
|
||
| let balance = dispatcher.get_balance(); | ||
| assert(balance == 0, 'balance == 0'); | ||
|
|
||
| dispatcher.increase_balance(100); | ||
|
|
||
| let balance = dispatcher.get_balance(); | ||
| assert(balance == 100, 'balance == 100'); | ||
| } | ||
| {{#include ../../listings/snforge_overview/crates/using_cheatcodes/tests/caller_address/failing.cairo}} | ||
| ``` | ||
|
|
||
| However, when running this test, we will get a failure with a message | ||
| This test passes, which means that `increase_balance` method panics with expected message. |
There was a problem hiding this comment.
I've looked at the whole document again and I think this is confusing to have it pass. If that's necessary you can add hidden should_panic but we should be showing to the users that this fails so it's obvious what's the point of using a cheat there.
Comment on lines
+74
to
+75
| using [`stop_cheat_caller_address`](../appendix/cheatcodes/caller_address.md#stop_cheat_caller_address). | ||
| We will demonstrate its behavior using `SafeDispatcher` to show when exactly the fail occurs: |
There was a problem hiding this comment.
Same here, this should just fail (or at least appear as failed to the user).
|
I think you can also link this issue #1671 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack:
docs/listingsto CI #2471docs/listings#2470 ⬅