Compile contract artifacts in test targets #1585
Conversation
maciektr
commented
Sep 9, 2024
maciektr
commented
- Refactor starknet-contract compiler
- Emit starknet contracts in test targets
maciektr
force-pushed
the
maciektr/test-target
branch
5 times, most recently
from
e2cf31d
to
9619404
Compare
maciektr
changed the title
maciektr
requested review from
ddoktorski,
cptartur and
integraledelebesgue
| @@ -74,6 +82,45 @@ impl Compiler for TestCompiler { | |||
| )?; | |||
| } | |||
|
|
|||
| if starknet { | |||
| compile_contracts(test_crate_ids, target_dir, unit, db, ws)?; | |||
There was a problem hiding this comment.
Note, this does not search in all dependencies.
There was a problem hiding this comment.
As in https://foundry-rs.github.io/starknet-foundry/testing/contracts-collection.html?highlight=build-external#how-contracts-are-collected, Forge requires contracts defined in dependencies to be explicitly declared in manifest file, for them to be available in tests.
On the contrary, Cairo Test searches all dependencies (even not direct ones) for contracts and compiles all of them, without any explicit declaration. This might be sub-optimal in some cases, as contracts can be quite large (see: https://swmansion.slack.com/archives/C037B80988J/p1726501029482169).
this only searches for contracts in the main component of the compilation unit (so the package for unit tests, and the tests directory for integration tests) + ones declared in build-external-contracts.
| @@ -74,6 +82,45 @@ impl Compiler for TestCompiler { | |||
| )?; | |||
| } | |||
|
|
|||
| if starknet { | |||
| compile_contracts(test_crate_ids, target_dir, unit, db, ws)?; | |||
| }; | ||
| let external_contracts = external_contracts | ||
| .into_iter() | ||
| .chain(vec![format!("{package_name}::*")]) |
There was a problem hiding this comment.
What is the reason that this is not included earlier?
There was a problem hiding this comment.
The previous target is a unit test one, so the tested package is the main component of the compilation unit. This means, that it will be searched for contracts as a main package (i.e. it's in test_crate_ids).
For the integration ones (below), the tested package is treated as any other dependency, so it's not in test_crate_ids and thus would not be searched for contracts, unless we explicitly ask for this.
We also can assume, that build-external-contracts does not include this package already, as it's defined in starknet-contract target of said package, thus is external to the package in question.
There was a problem hiding this comment.
Thanks! As a nitpick this code can be placed further down for better readability, for example on line 702.
maciektr
force-pushed
the
maciektr/test-target
branch
from
e9efe0c
to
4839a41
Compare
|
|
||
| impl ContractArtifacts { | ||
| fn new( | ||
| package_name: &PackageName, |
There was a problem hiding this comment.
nit: You can just move the variable here, as in the only function you're using the ContractArtifacts::new, you don't move the package_name anywhere else.
| ) -> Self { | ||
| Self { | ||
| id: short_hash((&package_name, &contract_path)), | ||
| package_name: package_name.clone(), |
There was a problem hiding this comment.
If u decide to move package_name here, u will not have to clone it here
| let mut artifacts = StarknetArtifacts::default(); | ||
| let mut file_stem_calculator = ContractFileStemCalculator::new(contract_paths); | ||
|
|
||
| for (decl, class, casm_class) in izip!(contracts, classes, casm_classes) { |
There was a problem hiding this comment.
It's very nit, you can even ignore it, but this always catches my attention. Are You sure we want to use short trimmed names like decl instead of contract_declaration or declaration?
| let contract_name = decl.submodule_id.name(db.upcast_mut()); | ||
| let contract_path = decl.module_id().full_path(db.upcast_mut()); | ||
|
|
||
| let contract_selector = ContractSelector(contract_path); | ||
| let package_name = contract_selector.package(); |
There was a problem hiding this comment.
With the current implementation it seems that compiling a given conract with test and starknet-contract targets will produce artifacts with the same contract_name and package_name (and id as well?). Would it make sense to distinguish between these artifacts at this level, perhaps by appending unittest or integrationtest to the contract name?
There was a problem hiding this comment.
Actually, the contract names are not prepended by package name, but rather by the target name (
).
But technically it's true, that it can be overwritten! Thanks for pointing this out!
We only ensure uniqueness of pairs (target_name, target_kind), not target_name only - so if someone provides test targets manually in the manifest, it would be possible to make a conflict. Of course it's a very esoteric case, but still.
