Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: inject contract id into the namespace for tests automatically #3729

Merged
merged 7 commits into from
Jan 24, 2023

Conversation

kayagokalp
Copy link
Member

@kayagokalp kayagokalp commented Jan 9, 2023

closes #3673.

About this PR

This PR adds CONTRACT_ID injection while building contracts with forc build --test. CONTRACT_ID is the id of the contract without the tests added. To find out that id, we first compile the contract without tests enabled.

The rough outline of stuff happening here:

  1. We iterate over BuildPlan members to find out contracts and collect their contract id into an injection map. In this step only the contracts are built. To determine contract id we need to compile the contract without tests. Since we also need the bytecode of the contract without tests, we are collecting them as we come across them while iterating over members.
  2. With the injection map build and execute all the tests, so basically after first step we are just doing the old forc-test behaviour.

So basically I added a pre-processing step for contract id collection for those members that require it (contracts).

This enables the following test structure:

let caller = abi(MyContract, CONTRACT_ID);
let result = caller.test_function {}();
assert(result == true)

TODO

  • Documentation

@kayagokalp kayagokalp self-assigned this Jan 9, 2023
@kayagokalp kayagokalp added enhancement New feature or request forc-test Everything related to the `forc-test` lib and `forc test` command. labels Jan 9, 2023
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
@kayagokalp kayagokalp marked this pull request as ready for review January 10, 2023 20:40
@kayagokalp kayagokalp requested a review from a team January 10, 2023 20:41
@kayagokalp kayagokalp enabled auto-merge (squash) January 10, 2023 20:51
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Excited for this!

That said, I don't think we should be distinguishing between contract and non-contract in forc-pkg itself like this, as the difference is not particularly relevant to forc-pkg and is only a concern of forc-test.

We should aim to make the minimum changes possible in forc-pkg. Namely, we should only need to add the ability to optionally add an item to a package's namespace when building it (so that we can add the CONTRACT_ID).

Rather than having forc-pkg special-case contracts, I think we should instead do something like the following:

  1. In forc-test, rather than building the whole workspace, iterate over members (in the correct order) and only build the contract members without tests.
  2. Add an option to forc-pkg build functionality that allows for injecting item(s) into a package's namespace.
  3. In forc-test, rather than building the whole workspace with tests at once, iterate over the members (in the correct order) and inject the CONTRACT_ID into contract members as necessary.

This might end up being slightly more verbose, but should help to keep a clearer separation of concerns between forc-pkg and forc-test. Thoughts?

@kayagokalp
Copy link
Member Author

I wasn't really happy with how this turned out tbh, thanks for the comment @mitchmindtree! I think the proposal seems it would be a lot cleaner. I will come-up with the implementation tomorrow morning 👍

@kayagokalp kayagokalp marked this pull request as draft January 14, 2023 22:48
auto-merge was automatically disabled January 14, 2023 22:48

Pull request was converted to draft

kayagokalp added a commit that referenced this pull request Jan 18, 2023
…-pkg` (#3770)

This PR, refactor the `BuildPlan` generation a little to be able to
re-use some of the parts from other crates that requires to create
`BuildPlan` from build opts.

On top of that this PR also introduces a way to inject list of items
into the namespace of any member of the workspace.

This is done to unblock #3729

closes #3763.
@kayagokalp
Copy link
Member Author

This should be good to go for another review, I will be introducing a general refactor to forc-test to address some of the left over review comments from earlier PRs.

@kayagokalp kayagokalp marked this pull request as ready for review January 20, 2023 14:42
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Nice, this new approach is looking much better! Just a few nits we should address first.

docs/book/src/testing/unit-testing.md Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-test/src/lib.rs Outdated Show resolved Hide resolved
forc-test/src/lib.rs Outdated Show resolved Hide resolved
forc-test/src/lib.rs Outdated Show resolved Hide resolved
forc-test/src/lib.rs Show resolved Hide resolved
Co-authored-by: mitchmindtree <mitchell.nordine@fuel.sh>
@kayagokalp kayagokalp marked this pull request as draft January 23, 2023 12:38
@kayagokalp kayagokalp marked this pull request as ready for review January 23, 2023 14:08
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Nice, exciting stuff! 🚀

I've left a couple of optional suggestions, but none are blockers.

forc-pkg/src/pkg.rs Show resolved Hide resolved
forc-test/src/lib.rs Show resolved Hide resolved
forc-test/src/lib.rs Show resolved Hide resolved
@kayagokalp kayagokalp requested a review from a team January 24, 2023 10:16
@kayagokalp kayagokalp enabled auto-merge (squash) January 24, 2023 10:16
@kayagokalp kayagokalp merged commit 76606cd into master Jan 24, 2023
@kayagokalp kayagokalp deleted the kayagokalp/3673 branch January 24, 2023 10:32
sdankel pushed a commit that referenced this pull request Jan 25, 2023
…3729)

closes #3673.

## About this PR

This PR adds `CONTRACT_ID` injection while building contracts with `forc
build --test`. `CONTRACT_ID` is the id of the contract without the tests
added. To find out that id, we first compile the contract without tests
enabled.

The rough outline of stuff happening here:

1. We iterate over `BuildPlan` members to find out contracts and collect
their contract id into an injection map. In this step only the contracts
are built. To determine contract id we need to compile the contract
without tests. Since we also need the bytecode of the contract without
tests, we are collecting them as we come across them while iterating
over members.
2. With the injection map build and execute all the tests, so basically
after first step we are just doing the old `forc-test` behaviour.

So basically I added a pre-processing step for contract id collection
for those members that require it (contracts).

This enables the following test structure:

```rust
let caller = abi(MyContract, CONTRACT_ID);
let result = caller.test_function {}();
assert(result == true)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request forc-test Everything related to the `forc-test` lib and `forc test` command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inject CONTRACT_ID automatically while testing contracts
3 participants