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

test: add isolate mode JS integration test #652

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

agostbiro
Copy link
Member

There is a Rust Solidity test runner integration test that tests that the isolate flag works by checking the gas report. I copied the same test i to the JS integration test to make sure that the enabling isolate mode works from JS as well.

I've also abstracted out the JS Solidity test runner integration test context to be able to be used from multiple test source files.

Copy link

changeset-bot bot commented Sep 9, 2024

⚠️ No Changeset found

Latest commit: c372165

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@agostbiro agostbiro self-assigned this Sep 9, 2024
@agostbiro agostbiro temporarily deployed to github-action-benchmark September 9, 2024 17:22 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark September 9, 2024 17:22 — with GitHub Actions Inactive
@agostbiro agostbiro added the no changeset needed This PR doesn't require a changeset label Sep 9, 2024
@agostbiro agostbiro added this to the Solidity tests, phase 2 milestone Sep 9, 2024
Copy link
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

I understand that the test contract is a copy of an existing solidity test, but I think we can do something much simpler, if we only care about testing that isolate mode is enabled:

contract IsolateTest is Test {
    StorageLib storageLib;

    function setUp() public {
        storageLib = new StorageLib();
    }

    function testIsolateTest() public {
        // tstore key: 1 with value :2
        storageLib.tstore(1, 2);

        // toload key: 1
        uint256 val = storageLib.tload(1);

        // If the test is run with `--isolate` flag, the value should be 0
        // as --isolate run each top level call as seperate transaction, so tload will return 0
        assertEq(val, 0, "did you forget to use --isolate flag for 'forge test'?");
    }
}

contract StorageLib {
    function tstore(uint256 key, uint256 val) public {
        assembly {
            tstore(key, val)
        }
    }

    function tload(uint256 key) public view returns (uint256 val) {
        assembly {
            val := tload(key)
        }
        return val;
    }
}

(copied from https://github.com/pancakeswap/pancake-v4-core/blob/857b4bf8055e74465900b5c15997fa100ee3044a/test/Isolate.t.sol)

I'd rather use this simpler version because it's easier to understand what is testing and why it works.

readonly artifacts: Artifact[];
readonly testSuiteIds: ArtifactId[];

constructor(artifacts: Artifact[], testSuiteIds: ArtifactId[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Normally a static async constructor means a private constructor, unless you want to allow creating instances directly too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks fixed in 4f96f26

@agostbiro
Copy link
Member Author

@agostbiro agostbiro temporarily deployed to github-action-benchmark September 10, 2024 11:01 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark September 10, 2024 11:01 — with GitHub Actions Inactive
Base automatically changed from test/js-unit-tests to feat/solidity-tests September 10, 2024 12:53
import "./test.sol";
import "./Vm.sol";

contract IsolateModeTest is DSTest {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a mention here that this was taken from the Pancakeswap repo.

Copy link
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

Minor comment, feel free to merge after fixing that.

@fvictorio fvictorio temporarily deployed to github-action-benchmark September 12, 2024 10:30 — with GitHub Actions Inactive
@fvictorio fvictorio temporarily deployed to github-action-benchmark September 12, 2024 10:30 — with GitHub Actions Inactive
@agostbiro agostbiro merged commit aa34d48 into feat/solidity-tests Sep 12, 2024
36 of 38 checks passed
@agostbiro agostbiro deleted the test/isolate-mode-test branch September 12, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changeset needed This PR doesn't require a changeset
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants