Skip to content

Conversation

@cptartur
Copy link
Member

Closes #

Introduced changes

  • Split contents of contract.md into multiple sections
  • Added a section on using external contracts. Until now it was buried in the docs on tests collection (left a mention of it there as well).

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@cptartur cptartur requested a review from a team as a code owner May 21, 2025 12:53
@cptartur cptartur requested review from ddoktorski and kkawula May 21, 2025 12:53
# `snforge` Overview

* [Running Tests](testing/running-tests.md)
* [Writing Tests](testing/testing.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is called the same as sub section

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed

Copy link
Contributor

Choose a reason for hiding this comment

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

[dev-dependencies]
snforge_std = "0.44.0"

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed here and in other place as well

Sometimes the test code failing can be a desired behavior.
Instead of manually handling it, you can simply mark your test as `#[should_panic(...)]`.
[See here](./testing.md#expected-failures) for more details.
This chapter shows how to test smart contracts using Starknet Foundry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we shorten this entire paragraph? It sounds a little clunky

Copy link
Member Author

Choose a reason for hiding this comment

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

I've shortened it


Note that the name after `mod` will be used as the contract name for testing purposes.

## Writing Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

This heading is duplicated and doesn't look good for me

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed the top level heading, I think it makes a bit more sense now

@cptartur cptartur requested a review from kkawula May 26, 2025 13:23
Comment on lines +3 to +5
Sometimes we want to test contracts functions that can panic, like testing that function that verifies caller address
panics on invalid address. For that purpose Starknet also provides a `SafeDispatcher`, that returns a `Result` instead of
panicking.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Sometimes we want to test contracts functions that can panic, like testing that function that verifies caller address
panics on invalid address. For that purpose Starknet also provides a `SafeDispatcher`, that returns a `Result` instead of
panicking.
Sometimes we want to test contract functions that may panic. For instance, an example of this would be a function which verifies caller address and panics when it encounters an invalid one. For that purpose Starknet provides `SafeDispatcher` that returns a `Result` instead of
panicking.


## `SafeDispatcher`

Using `SafeDispatcher` we can test that the function in fact panics with an expected message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Using `SafeDispatcher` we can test that the function in fact panics with an expected message.
By using `SafeDispatcher`, we can test that the function in fact panics with an expected message.

Sometimes the test code failing can be a desired behavior.
Instead of manually handling it, you can simply mark your test as `#[should_panic(...)]`.
[See here](../contracts/handling-errors.md#expecting-test-failure) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[See here](../contracts/handling-errors.md#expecting-test-failure) for more details.
See [here](../contracts/handling-errors.md#expecting-test-failure) for more details.


## Add a Dependency

First, add a dependency on the contract package, either in `Scarb.toml` directly or by using `scarb add packageName`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Cairo packages are named with snake_case 😅 :

Suggested change
First, add a dependency on the contract package, either in `Scarb.toml` directly or by using `scarb add packageName`.
First, add a dependency on the contract package, either in `Scarb.toml` directly or by using `scarb add package_name`.


```toml
[[target.starknet-contract]]
build-external-contracts = ["externalPackage1::Contract1", "otherExternalPackage::path::to::Contract2"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above:

Suggested change
build-external-contracts = ["externalPackage1::Contract1", "otherExternalPackage::path::to::Contract2"]
build-external-contracts = ["external_package::Contract1", "other_external_package::path::to::Contract2"]

* [Writing Tests](testing/testing.md)
* [Test Attributes](testing/test-attributes.md)
* [Testing Smart Contracts](testing/contracts.md)
* [Writing Contracts Tests](testing/contracts/writing_tests.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* [Writing Contracts Tests](testing/contracts/writing_tests.md)
* [Writing Contract Tests](testing/contracts/writing_tests.md)

@@ -0,0 +1,50 @@
# Writing Contracts Tests

## The Test Contract
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## The Test Contract
## Test Contract

Comment on lines +35 to +47
Collected 2 test(s) from testing_smart_contracts_handling_errors package
Running 2 test(s) from tests/
[FAIL] testing_smart_contracts_handling_errors_integrationtest::panic::failing

Failure data:
(0x50414e4943 ('PANIC'), 0x444159544148 ('DAYTAH'))

[PASS] testing_smart_contracts_handling_errors_integrationtest::handle_panic::handling_string_errors (l1_gas: ~0, l1_data_gas: ~96, l2_gas: ~280000)
Running 0 test(s) from src/
Tests: 1 passed, 1 failed, 0 skipped, 0 ignored, 0 filtered out

Failures:
testing_smart_contracts_handling_errors_integrationtest::panic::failing
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect output for the example above

Failure data:
(0x50414e4943 ('PANIC'), 0x444159544148 ('DAYTAH'))

[PASS] testing_smart_contracts_handling_errors_integrationtest::handle_panic::handling_string_errors (l1_gas: ~0, l1_data_gas: ~96, l2_gas: ~280000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We show in the code snippet only the failing function test, so ideally the output shouldn't contain other tests

```rust
{{#include ../../../listings/testing_smart_contracts_handling_errors/tests/handle_panic.cairo}}
```
You also could skip the de-serialization of the `panic_data`, and not use `try_deserialize_bytearray_error`, but this way you can actually use assertions on the `ByteArray` that was used to panic.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You also could skip the de-serialization of the `panic_data`, and not use `try_deserialize_bytearray_error`, but this way you can actually use assertions on the `ByteArray` that was used to panic.
You could also skip deserializing the `panic_data` and avoid using `try_deserialize_bytearray_error`, but in that case `panic_data` remains in its raw format as an array of felts.

@github-actions
Copy link

github-actions bot commented Jul 4, 2025

Hi! This pull request hasn't had any activity for a while, so I am
marking it as stale. It will close in 14
days if it is not updated. Thanks for contributing!

@github-actions github-actions bot added the stale Stale Bot label Jul 4, 2025
@github-actions
Copy link

This pull request has been automatically closed due to inactivity.
Feel free to reopen it or create a new one if needed.
Thanks for contributing!

@github-actions github-actions bot closed this Jul 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale Bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants