alexfertel / bulloak Goto Github PK

Foo
└── It reverts when X.

Running bulloak scaffold Foo.tree results in an error: bulloak error: invalid identifier: ..

I understand it’s because bulloak sees the word ‘when’ in the leaf name and thinks it’s a condition instead of an action.

While I understand I could write:

Foo
└── When X
    └── It reverts.

I don’t like that because then the action statement becomes a comment which is not checked with bulloak check if removed.

Besides, there are legitimate usecases for the word ‘when’ used to describe the action.

Expected behavior: The word ‘when’ has a special meaning only at the beginning of a leaf name.

I would expect ./**/*.tree to match all subdirectories, but I had to do ./**/**/**/*.tree to capture a file at the path ./test/perps-market-manager/create-market/createMarket.tree https://github.com/sense-finance/perps-v1/pull/15/commits/6fb8051af25202f8a47a3ab092491c159385efa6

Currently, the compiler doesn't need much optimization: on Sablier's codebase, which to my knowledge, is the largest one using BTT (58 trees), it shows the following metrics:

Time (mean ± σ):      21.6 ms ±   9.0 ms    [User: 3.2 ms, System: 4.1 ms]
Range (min … max):    11.5 ms …  58.5 ms    83 runs

Which is instantaneous. However, it would be nice to have a set of benches to spot regressions and improvements as the codebase grows.

This is a half-baked idea, but the thinking is:

• I write a .tree file and bulloak -w <file>
• I edit my .tree file, now my test is out of date
• I have to have generate new solidity with bulloak and manually move things

I'm picturing a bulloak check --fix or bulloak update command that, if I only added or moved lines in the tree, regenerates my test file with the code properly moved also. If lines were also changed, perhaps what it can do is something like this:

contract MyTest is Test {
  function test_123() {
    // bulloak automatically scaffolded everything and populated what it could
  }

  // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  // ======== BULLOAK AUTOGENERATED SEPARATOR ========
  // >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
  // Code below this section could not be automatically moved by bulloak and must be manually moved.

  function test_456() {
    // bulloak never deletes your code so it places it here
  }
}

Features:

• Syntax highlighting for *.tree files better than what is offered by Tree (e.g. different colors for "when" and "given")
• Parse the tree nodes and check that all start with "when", "given", or "it"
• Continuously check that the implementation matches the spec (with a setting to turn this behavior on and off)
• Run scaffold as a command (i.e. via CMD+SHIFT+P) with two options:
  • Run on one file alone
  • Run on entire code base
• Right-clicking the contents of any *.tree file should contain an option to scaffold

I imagine that this is an idea for the long term and that it may be overkill at the moment.

ContractName::func1
└── It should have a name mismatch in the contracts.

MismatchedContractName::func2
└── It should have a name mismatch in the contracts.

For the above tree, we get the following message when running bulloak check:

$ cargo run -- check tests/check/contract_names_mismatch_multiple_roots.tree
warn: an error occurred while parsing the tree: contract name mismatch: expected 'ContractName', found 'MismatchedContractName'
   --> tests/check/contract_names_mismatch_multiple_roots.tree

warn: 1 check failed

We should make the violation fixable and implement the auto fix.

To reproduce:

• Create a .tree file that's empty
• Run bulloak -w ./**/*.tree
• Get the below output:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /Users/mds/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bulloak-0.4.6/src/scaffold/parser.rs:202:58
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Bulloak should exit without panicking here, and should print an error similar to that when the contents of the .tree file are invalid

Given the following foundry.toml:

[fmt]
bracket_spacing = true

running

bulloak check test/Foo.tree --fix

reformats the entire test/Foo.t.sol file because it doesn’t apply custom fmt settings from foundry.toml.

Discussed in #77

             ...
            └── given all streams are warm
            ├── when the caller is unauthorized for all streams
            │  ├── when the caller is a malicious third party
            │  │  └── it should revert
            │  └── when the caller is the recipient
            │     └── it should revert
            ├── when the caller is unauthorized for some streams
            │  ├── when the caller is a malicious third party
            │  │  └── it should revert
            │  └── when the caller is the recipient
            │     └── it should revert
            ...

function test_RevertWhen_CallerUnauthorizedAllStreams_MaliciousThirdParty()
        external
        whenNotDelegateCalled
        whenArrayCountNotZero
        givenNoNull
        givenAllStreamsWarm
        whenCallerUnauthorized
    

This a bit nitpicky (hope you don't mind all my recent comments 😅), but I always write comments as full sentences. Bulloak is currently case sensitive, e.g. It is not recognized as a keyword but it is. I tried to write my spec as It should do x. so the resulting comment in the test file is a full sentence (starts with capital letter and ends with a period).

I see two ways to support this:

1. Make the keywords case insensitive and copy them as-is into the test.
2. Keep them as case-sensitive, and automatically convert the requirement into a sentence by (when necessary) capitalizing the first letter or adding a period. Different users have different preferences so this would require a new CLI flag

This is a simple change so I'm happy to PR this one in. I think option 1 is probably the way to go here to keep the interface simple, but curious to hear your thoughts and if you're open to this.

The it should revert keyword is ignored if there is a dependant leaf comment on the branch, which causes the translated test methods to be missing the RevertWhen.

Context:

FooTest
└── When stuff is called // Comments are supported.
    └── When a condition is met
        └── It should revert.
            └── Because we shouldn't allow it.

Expected behaviour:

    function test_RevertWhen_PassingAnEmptyGreetingString() external {
        // it should revert
        //     Because we shouldn't allow it.
    }

Actual behaviour:

    function test_WhenPassingAnEmptyGreetingString() external {
        // it should revert
        //      Because we shouldn't allow it.
    }

Ok, here is the weirdest bug report I’ve ever submitted 😄

Say I have the following tree:

Foo
├── when a
│   └── it X
├── when b
│   └── it Y
└── when the method is called a second time
    └── it Z

bulloak scaffold ./test/Foo.tree -w writes the following file at ./test/Foo.t.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.0;

contract Foo {
    function test_WhenA() external {
        // it X
    }

    function test_WhenB() external {
        // it Y
    }

    function test_WhenTheMethodIsCalledASecondTime() external {
        // it Z
    }
}

So far so good.

Now say requirements change and now my tree looks like this:

Foo
├── when b
│   └── it Y
└── when a
    └── it X

Notice, two things change in the tree:

1. when b moves above when a
2. when the method is called a second time gets removed

I run bulloak check ./test/Foo.tree --fix and get the following error:

thread 'main' panicked at /home/john/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bulloak-0.6.3/src/check/violation.rs:418:33:
should parse solidity string: [Diagnostic { loc: File(0, 734, 735), level: Error, ty: ParserError, message: "unrecognised token '}', expected \"(\"", notes: [] }]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

If I don’t reorder when b and when a, bulloak does not panic. So bulloak check ./test/Foo.tree --fix with the following tree works:

Foo
├── when a
│   └── it X
└── when b
    └── it Y

Here comes the surprising part: when I do reorder when a and when b statements, bulloak check only panics if the removed when statement has TheMethod string in it.

Given a modified tree:

Foo
├── when b
│   └── it Y
└── when a
    └── it X

Solidity test files bulloak check works with correctly:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.0;

contract Foo {
    function test_WhenA() external {
        // it X
    }

    function test_WhenB() external {
        // it Y
    }

-    function test_WhenTheMethodIsCalledASecondTime() external {
+    function test_WhenC() external {
        // it Z
    }
}

Solidity test files bulloak check panics with:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.0;

contract Foo {
    function test_WhenA() external {
        // it X
    }

    function test_WhenB() external {
        // it Y
    }

-    function test_WhenTheMethodIsCalledASecondTime() external {
+    function test_WhenTheMethod() external {
        // it Z
    }
}

What seems to be causing trouble is TheMethod part of the last test case name.

bulloak version 0.6.3

I would be nice to commit your Cargo.lock file, as suggested by the Cargo team. This also would help with creating a nix derivation for this utility, which requires a Cargo.lock file for reproducibility.

From this discussion and later this PR, we want to add support for using the given keyword instead of when.

It would just be a substitution, so we only have to add given as a keyword and handle it the same way we would handle when.

Assume that I want to have the following .tree

WhenTest
└── given blax1 and blax2 are not met
    ├── when blax3 is not met
    │   └── it shouldn't be advanceable
    └── given blax3 is met
        ├── when blax4 is met
        │   └── it shouldn't be advanceable
        └── when blax4 is not met
            └── it should be advanceable

This provides a great structure. Though, after using bulloak, this puts modifiers on my test functions. The problem is I can't actually do anything inside the modifier, because at that time(inside modifier), I don't have the contract and I can only access it inside my test function. So Not sure what to do. Modifiers actually help with readability of the test function description which is great, but If I can't do anything inside modifier, then whatever message modifier has, I have to do it inside the test, which is misleading for sure. Hope I am making sense.

I tried not emitting modifiers(by -m flag), I also tried removing all given keywords inside .tree, but it still puts modifiers on the functions. Even if there's a way not to put the modifier on the function, then test function loses the description.

I had the following experience with bulloak recently:

At some point during a protocol development, we started outlining trees for methods.
Once finished, we scaffolded test files with over 100 test cases.

When we ran tests, it reported all tests succeed even though majority of them were empty tests generated with bulloak.

We manually added vm.skip(true); to over a 100 test cases to have an accurate reporting on how many tests actually pass and how many are waiting to be implemented (skipped).

Then, as we implemented each test, we removed the vm.skip(true); statement.

I believe it would be cool if bulloak could add vm.skip(true); to all scaffolded tests or at least provide a flag to do that.

Depending upon the contents of the *.tree file, the scaffolded Solidity code can end up somewhat verbose. It would be nice if there were a way to shorten the generated text using a LLM like GPT.

E.g. given this input:

├── when the deposit amount is greater than a specific value
│   └── it should revert
└── when the deposit amount is less than or equal to the specific value
    └── it should do smth

The following function names would be generated:

function test_RevertWhen_DepositAmountGreaterThanSpecificValue {}
function test_DepositAmountLessThanOrEqualToSpecificValue {}

Notice that the "the" and the "is" and the "a" terms have been removed.

Alternatively:

function test_RevertWhen_DepositAmountNotGT {}
function test_DepositAmountLTE {}

Background

Bulloak treats test contracts independently from one another. This design choice necessitates defining modifiers within the same file as the test contract. However, in large projects where modifiers are shared among tests, this contradicts the principle of DRY.

Feature Request

• Introduce an option to disregard modifier definitions in the test contract. This can be achieved by including a flag, such as --ignore-modifiers.

• Bulloak should still include the modifier names in test functions, but it should not strictly require them to be defined in the same file.

Currently, Bulloak is shipped under the tag category. It would be great to see it shipped as a Software Release.

We overwrite the generated file if it exists. Gate this behavior behind a CLI flag.

I have a function that looks like this:

/**
 * @dev Generates pseudo-randomly a salt value using a diverse selection of block and
 * transaction properties.
 * @return salt The 32-byte pseudo-random salt value.
 */
function _generateSalt() internal view returns (bytes32 salt) {
    salt = keccak256(
        abi.encode(
            blockhash(block.number - 32),
            block.coinbase,
            block.number,
            block.timestamp,
            block.prevrandao,
            block.chainid,
            msg.sender
        )
    );
}

The rationale is add a lot of block properties to make it as unique as possible across chains/txs. But this isn't all possible properties, so a spec might look like this:

CreateX_GenerateSalt_Internal_Test
├── It never reverts.
└── It should be a function of multiple block properties and the caller.

But that's not fully specified as it doesn't specify which block properties. So it might be nice to support something like this:

CreateX_GenerateSalt_Internal_Test
├── It never reverts.
└── It should be a function of the below block properties and the caller:
    ├── - blockhash(block.number - 32),
    ├── - block.coinbase,
    ├── - block.number,
    ├── - block.timestamp,
    ├── - block.prevrandao,
    ├── - block.chainid,
    └── - msg.sender.

Which would get rendered into a test like this:

function test_ShouldBeAFunctionOfMultipleBlockPropertiesAndTheCaller() external {
  // It should be a function of multiple block properties and the caller.
  // The full set of dependencies is:
  //    - blockhash(block.number - 32),
  //    - block.coinbase,
  //    - block.number,
  //    - block.timestamp,
  //    - block.prevrandao,
  //    - block.chainid,
  //    - msg.sender.
}

In other words, the rule here is: Anything nested under an it gets included in the comments for that it, and it gets included at the respective indent level. So since there's one level of nesting for all those bullet points, in the comment they are indented by two spaces. As another example:

CreateX_GenerateSalt_Internal_Test
├── It never reverts.
└── It should be a function of the below block properties and the caller:
    ├── Here is some text
    │   ├── More examples
    │   └── Bulloak is great
    ├── Add block.chainid,
    └── Add msg.sender

would generate:

function test_ShouldBeAFunctionOfMultipleBlockPropertiesAndTheCaller() external {
  // It should be a function of the below block properties and the caller:
  //   Here is some text
  //     More examples
  //     Bulloak is great
  //   Add block.chainid,
  //   Add msg.sender
}

I went down the BTT some time in the past while researching and contributing to the Sablier V2 code base and noticed that test files generated by Bulloak would have different test contract names than those of Sablier V2.

It could be helpful to devs to add tips in README.md on how to easily rename generated test contracts in VSCode with a simple regex search and replacement as follows:

Example:

A .tree file employing Sablier V2 format for the test label and filename: functionToTest.t.sol.

functionToTest.t.sol
├── Given one condition
│   └── It should never revert
└── Given another condition
    └── It should succeed

Search: (\w+)tsol
Replace: \u$1_Unit_Concrete_Test or \u$1_Integration_Concrete_Test

Executing the above search and replacement would rename the Bulloak generated test contract name from functionToTesttsol to FunctionToTest_Unit_Concrete_Test or FunctionToTest_Integration_Concrete_Test respectively.

Add a way to support variables which should be fuzzed

This could be:

• have new keywords parsed in the .tree (between, greater than, lesser than, equals for the opposite/reverting)
• add arguments to the function signature, based on these
• create a range for this values (using forge-std bound)

For instance

Test.Test
├── When X is between a and b 
└──── it should do something

should then have a corresponding test

function test_WhenXIsBetweenAAndB(uint256 fuzzedX) external {
    fuzzedX = bound(fuzzedX, A, B);
    // It should do something

This is more like a nitpick, but properly formatted *.tree file should have:

• Three spaces before beginning of new line and ├──
• Three spaces before beginning of new line and └──
• Two spaces in between │ and ├──
• Two spaces in between │ and └──
• Two spaces in between │ and │

Here's an example of a properly formatted tree:

RefundableAmountOf_Integration_Concrete_Test
├── given null
│  └── it should revert
└── given not null
   ├── given balance zero
   │  └── it should return zero
   └── given balance not zero
      ├── given paused
      │  └── it should return correct refundable amount
      └── given not paused
         ├── when total debt exceeds balance
         │  └── it should return zero
         └── when total debt does not exceed balance
            └── it should return correct refundable amount

As is usually expected in the case of forge tests :)

Currently, Bulloak works with .tree files that target a single specific function. This is great for a small number of contracts or a subset of functionality to be tested but doesn't scale when there are lots of contracts and functions.

Ideally, it should be possible to scaffold on a per-contract basis, targeting multiple functions in a single .tree file that is emitted to the same test contract. Consider the following ERC20 tree as an example:

ERC20.t.sol
├── when decimals is called
│   └── it should return the decimals
├── when name is called
│   └── it should return the name
├── when symbol is called
│   └── it should return the symbol
├── when totalSupply is called
│   └── it should return the total supply
├── when balanceOf is called
│   └── it should return the balance of the address
├── when allowance is called
│   └── it should return the allowance of the owner to the spender
├── when mint is called
│   ├── when the caller does not have the minter role
│   │   └── it should revert
│   └── when the caller has the minter role
│       ├── given the address is the zero address
│       │   └── it should revert
│       └── given the address is not the zero address
│           ├── given the amount causes the total supply to overflow
│           │   └── it should revert
│           └── given the amount does not cause the total supply to overflow
│               ├── it should mint the tokens
│               ├── it should update the total supply
│               └── it should emit a {Transfer} event
├── when burn is called
│   ├── when the caller does not have sufficient balance
│   │   └── it should revert
│   └── when the caller has sufficient balance
│       ├── it should burn the tokens
│       ├── it should update the total supply
│       └── it should emit a {Transfer} event
├── when transfer is called
│   ├── given the address is the zero address
│   │   └── it should revert
│   └── given the address is not the zero address
│       ├── when the sender does not have sufficient balance
│       │   └── it should revert
│       └── when the sender has sufficient balance
│           ├── it should transfer the tokens
│           ├── it should emit a {Transfer} event
│           └── it should return true
├── when approve is called
│   ├── given the address is the zero address
│   │   └── it should revert
│   └── given the address is not the zero address
│       ├── it should set the allowance
│       ├── it should emit an {Approval} event
│       └── it should return true
├── when transferFrom is called
│   ├── given the from address is the zero address
│   │   └── it should revert
│   └── given the from address is not the zero address
│       ├── given the to address is the zero address
│       │   └── it should revert
│       └── given the to address is not the zero address
│           ├── given the amount is zero
│           │   └── it should revert
│           └── given the amount is not zero
│               └── when the spender is not the from address
│                   ├── when the spender has maximum allowance
│                   │   ├── it should transfer the tokens
│                   │   ├── it should emit a {Transfer} event
│                   │   └── it should return true
│                   └── when the spender does not have maximum allowance
│                       ├── when the spender does not have sufficient allowance
│                       │   └── it should revert
│                       └── when the spender has sufficient allowance
│                           ├── it should decrement the allowance
│                           ├── it should transfer the tokens
│                           ├── it should emit a {Transfer} event
│                           └── it should return true

For maximum flexibility of the directory structure, it may also be useful to specify the trees to be emitted to the same test contract. For example, it may be desirable to have all .tree files within some parent trees directory, further organized into subdirectories (e.g., token/ERC20, libraries/Strings, vendor/AccessControl`), which can be arbitrarily combined to scaffold any number of test files.

Right now, arg parsed with clap are passed as individual values to the relevant fn, as there are only 4 of them for scaffold (w, f, s and S) and 2 for check.

As more option are introduced, an ever-growing list of function parameters isn't sustainable and should rather be part of a struct (leaving the cli arg versus Bulloak.toml discussion for #38 ).

let scaffolder = Scaffolder::new(&self.solidity_version, &self.with_vm_skip);

would become

let scaffolder = Scaffolder::new(&self.cfg);

It would be interesting to find a way to specify the coverage used while building a tree (and reflect it in the test contract name or top-level comment).

For instance, for the following function and coverage type, we'd scaffold 2 tests contracts which we could name Foo_Test_Branch and Foo_Test_FullPath

bool foo (bool x, bool y) {
    if (a && b) {
        return true;
    }
    return false;
}

based on the following coverages (amongst other possibilities):

• branch coverage: where we're assessing if we covered every result of every decision structure (tested both true and false in an if for instance)

flowchart TD
    A[foo] --> C{a && b}
    C -->|True| D[return true]
    C -->|False| E[return false]

with the tree

TestMe::foo(branch)
├── when a and b are true
│   └── it should return true
└── when a is false
    └── it should return false

• path coverage: where we're assessing if we are covering each flow within the function under test

flowchart TD
    A[foo] --> C{a}
    C -->|True| D{b}
    C -->|False| E{b}
    D -->|True| F(true)
    D -->|False| G(false)
    E -->|False| H(false)
    E -->|False| I(false)

with the following tree

TestMe::foo(full path)
├── given a is true
│   ├── when b is true
│   │   └── it should return true
│   └── when b is false
│       └── it should return false
└── given a is false
    ├── when b is true
    │   └── it should return false
    └── when b is false
        └── it should return false

Things that I would like to configure (and which Bulloak would apply to all Solidity code generated via scaffold):

• Indenting (4 spaces instead of 2): #40

• SPDX license

• compiler pragma as a string (e.g., >=0.8.19)

• GPT processing: #39

• Whether I want a bespoke name for the final leaf (ref #37)

• Whether I want one-test-per-it-branch or bundled-its (ref #7 (comment))
  #7 (reply in thread))
  Potentially helpful for other users:

• Case sensitivity (ref #20)

It would be nice if these settings could be kept in foundry.toml somehow, but I don't think Foundry allows custom configs.

I'm sure @mds1 and future users will find other use cases with this config file.

From #7

Currently, we emit all the modifiers first, and then all the test functions. We should emit the modifiers right before the function where they are used. Like this:

    modifier whenNotIncludedInMerkleTree() {
        _;
    }

    function test_RevertWhen_InvalidIndex() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidRecipient() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidAmount() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidMerkleProof() external whenNotIncludedInMerkleTree { }

    modifier whenIncludedInMerkleTree() {
        _;
    }

    function test_Claim() external whenIncludedInMerkleTree { }

The following tree:

applyAccruedFundingTest
├── it should call _updateFundingIndex
└── when funding index changes
    ├── it should emit AccruedFundingApplied event
    ├── it should update the lastFundingIndexApplied
    ├── when position is long
    │   ├── when funding index is greater than lastFundingIndexApplied
    │   │   └── it should increase the entry price
    │   └── when funding index is less than lastFundingIndexApplied
    │       └── it should decrease the entry price
    └── when position is short
        ├── when funding index is greater than lastFundingIndexApplied
        │   └── it should decrease the entry price
        └── when funding index is less than lastFundingIndexApplied
            └── it should increase the entry price

should error when scaffolding since we can't generate duplicated test names -- that's not valid solidity syntax. Right now, we scaffold and silently dedup the conditions.

Automatically convert from

VS Code has a nice tree generator: Ascii Tree Generator plug in, but even using that can be a bit tedious converting to and from tree syntax. Bulloak doing this conversion for you would be quite nice.

The plugin allows you to use hashtags for tree structures and automatically converts it into the nice tree.

# when there is a condition
## it should revert
# when there isn't the condition
## when sender is authorized
### it should update the state

Hey @alexfertel thank you for taking the time to develop bulloak its very much apprecaited.

This is more of a question/issue.

Take the following tree

MyErc20::constructor
└── when contructed
    ├── it deploys
    ├── it sets the owner as sender
    └── it sets the name

Bulloak wants the test to be Called MyErc20, but the Contract we are testing is called MyErc20

If I call my test contract UnitMyErc20 bulloak will report on check

contract "MyErc20" is missing in .sol -- found "UnitMyErc20" instead

What is the best course of action here?

Currently, when passing option -w, we write to the file system in a file with the same name as the .tree, but with a .sol extension, i.e. foo.tree -> foo.sol. This means that we don't take into account the root node's filename at all. We should.

The above is handled in src/main.rs like this:

if config.write_files {
    let mut path = file.clone();
    path.set_extension("sol");
    fs::write(path, code)?;
}

We should instead improve the library's API so that we can query the filename and then handle I/O in main. Maybe adding an object that holds such data instead of returning the emitted code.

According to the documentation, bulloak check should verify if the test file matches the spec. However, it doesn't seem to be working for me :(

When I try bulloak check in a file where the test file doesn’t match the spec, it returns the following:

You can check this repo to see what I mean.

I have two .tree files: one with a simple example and another with the same example explained in the documentation, and they both returns the same.

Is there any specific configuration needed to make this work?

Currently we have a bug when there is more than one diagnosed error. bulloak's output is a bit wonky because of a missing newline:

•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••
bulloak error: found an action without conditions

├── it should match the output of a high-level hash
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

--- (line 2, column 1) ---•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••
bulloak error: found an action without conditions

└── it should never revert
^^^^^^^^^^^^^^^^^^^^^^^^^^

Context

In Sablier V2, we've applied this heuristic whereby the final leaves in the tree result in a Solidity test that bears the name of the function itself. Here's an example:

https://github.com/sablier-labs/v2-core/blob/52df5c15da76e47ca396fcf4986fefade0db237f/test/integration/concrete/comptroller/set-protocol-fee/setProtocolFee.t.sol#L43

The rationale is that the final leaf is in many ways the "happiest of all happy paths", i.e., what the developer hopes the user's txs will end up effecting in the vast majority of cases. It is thus helpful to give this leaf a privilege; separately, it may also be useful when visualizing a gas snapshot/ report.

Since tree files must begin with the name of the test contract (#3), the name of the function could be extracted from there by removing the Test suffix.

Input

Given the following tree:

SimpleBranchTest
├── when x is false
│   └── it should revert
└── when x is true
    └── it should return x

Output

I would like bulloak scaffold to produce the following output:

pragma solidity 0.8.0;

contract SimpleBranchTest {
  function test_RevertWhen_XIsFalse() external {
    // it should revert
  }
  
  modifier whenXIsTrue() {
       _;
  }

  function test_SimpleBranch() external whenXIsTrue {
    // it should return x
  }
}

Issue Description

PR #68 implemented the --skip-modifiers flag requested in #66, however it also seems to have introduced a bug.

When modifiers are skipped, Bulloak produces false positives. It reports that the test file implements all tree branches correctly, even when it doesn't.

Steps to Reproduce

Toggle the code below and try the following commands:

bulloak check ./recentAmountOf.tree --skip-modifiers # passes, but it shouldn't
bulloak check ./recentAmountOf.tree # fails, as it should

The first command passes, whereas the second fails - as it should because the test file contains test_BlahBlahBlah instead of test_WhenLastUpdatedTimeInPast.

RecentAmountOf_Integration_Concrete_Test
├── given null
│  └── it should revert
└── given not null
   ├── given paused
   │  └── it should return zero
   └── given not paused
      ├── when last updated time in present
      │  └── it should return zero
      └── when last updated time in past
         └── it should return the correct recent amount

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.22;

import { Integration_Test } from "../../Integration.t.sol";

contract RecentAmountOf_Integration_Concrete_Test is Integration_Test {
    function test_RevertGiven_Null() external {
        bytes memory callData = abi.encodeCall(flow.recentAmountOf, nullStreamId);
        expectRevert_Null(callData);
    }

    function test_GivenPaused() external givenNotNull {
        flow.pause(defaultStreamId);

        // It should return zero.
        uint128 recentAmount = flow.recentAmountOf(defaultStreamId);
        assertEq(recentAmount, 0, "recent amount");
    }

    function test_WhenLastUpdatedTimeInPresent() external givenNotNull givenNotPaused {
        // Update the last time to the current block timestamp.
        updateLastTimeToBlockTimestamp(defaultStreamId);

        // It should return zero.
        uint128 recentAmount = flow.recentAmountOf(defaultStreamId);
        assertEq(recentAmount, 0, "recent amount");
    }

    function test_BlahBlahBlah() external view givenNotNull givenNotPaused {
        // It should return the correct recent amount.
        uint128 recentAmount = flow.recentAmountOf(defaultStreamId);
        assertEq(recentAmount, ONE_MONTH_STREAMED_AMOUNT, "recent amount");
    }
}

CC @smol-ninja, @andreivladbrg.

Currently bulloak lets you go from spec (.tree file) to tests, and a really valuable feature would be to verify that the test architecture matches what's expected based on the tests. This is important to ensure that the spec stays up to date as tests change, for example by running bulloak check in CI which would exit with an error if they don't match.

Initial thoughts on what's considered a match:

• The tree file and test file should be in the same directory
• Their filenames can be anything as long as they match, e.g. MyContract.foo.tree and MyContract.foo.t.sol are the spec and test for the foo method.
• All modifier names and test names must be present
• The ordering of all modifiers and tests must match what's in the tree file
• Modifier and test function bodies do not need to match what bulloak generates (solidity verifies that modifier's have the _;)
• Any number of helper functions (functions that don't start with test, invariant, or statefulFuzz) are allowed in any order

One open question is how to handle fuzz/invariant tests here. Bulloak only generates concrete tests which I agree with. Then what I like to do is generate fuzz tests when applicable. For example given this spec:

foo.sol
 └── when stuff called
    └── it should revert

bulloak would generate:

pragma solidity [VERSION];

contract FooTest {
  modifier whenStuffCalled() {
    _;
  }

  function test_RevertWhen_StuffCalled() external whenStuffCalled {
    // it should revert
  }
}

And it's useful to be able to modify this to:

pragma solidity [VERSION];

contract FooTest {
  modifier whenStuffCalled() {
    _;
  }

  function test_RevertWhen_StuffCalled() external whenStuffCalled
  {
    // The concrete test calls the fuzz test with a hardcoded value
    test_RevertWhen_StuffCalled(100);
  }

  function testFuzz_RevertWhen_StuffCalled(uint256 x) external whenStuffCalled
  {
    // then the test logic goes here
  }
}

So perhaps the rule here should be that for fuzz tests, we require that the fuzz test name matches one of the bulloak generated names with the exception of the added Fuzz prefix, per the foundry best practices.

Stateful fuzz (invariant) tests are a bit more complex, and perhaps should have their own spec file? Curious to get @PaulRBerg thoughts here too

Hi there,

I don't know if this is the intended behavior, but I wanted to highlight it.

I would expect comments to work correctly regardless of their placement, whether before, after, or on new lines following the code. However, it seems not to work when placed a couple of lines before or after the tree.

// This comment doesn't work

// This comment works
HashPairTest
├── It should never revert.  // This comment also works
├── When first arg is smaller than second arg
│   └── It should match the result of `keccak256(abi.encodePacked(a,b))`.
└── When first arg is bigger than second arg
    └── It should match the result of `keccak256(abi.encodePacked(b,a))`.
// This comment works

// This comment doesn't work

This is particularly concerning because I'm receiving the following error:

•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••
bulloak error: found an empty tree
file: ./test/foo.tree

warn: Could not scaffold 1 files. Check the output above or run bulloak check, which might prove helpful.

Meanwhile, when running bulloak check, I receive:

All checks completed successfully! No issues found.

awesome repo btw, congrats!!!

Curently, we emit a [VERSION] placeholder for the solidity version of the contracts. It would be nice to pass that as a CLI arg for easy automation.

bulloak scaffold does not print any SPDX identifier, which leads to the following warning thrown by Solidity:

SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.

It should be safe to have Bulloak print a dummy UNLICENSED identifier:

// SPDX-License-Identifier: UNLICENSED

Discussed in #18

function _efficientHash(bytes32 a, bytes32 b) internal pure returns (bytes32 hash) {
    assembly ("memory-safe") {
        mstore(0x00, a)
        mstore(0x20, b)
        hash := keccak256(0x00, 0x40)
    }
}

MyContract._efficientHash.sol
├── it should match the output of a high-level hash
└── it should never revert

•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••
bulloak error: found an action without conditions

├── it should match the output of a high-level hash
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

--- (line 2, column 1) ---•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••
bulloak error: found an action without conditions

└── it should never revert
^^^^^^^^^^^^^^^^^^^^^^^^^^

Instructions:

• Stop erroring in the semantic analysis phase when we find an Ast::Action while visiting the root.

  bulloak/src/semantics.rs

  Lines 157 to 160 in 3b4c0dc

  	Ast::Action(action) => {
  	self.errors
  	.push(self.error(action.span, ErrorKind::ActionWithoutConditions));
  	}

• Add support for emitting a test that has no parent condition.
  • This one probably requires a non-trivial refactor.

    bulloak/src/emitter.rs

    Lines 271 to 280 in 3b4c0dc

    	fn visit_action(&mut self, action: &ast::Action) -> result::Result<Self::Output, Self::Error> {
    	let mut emitted = String::new();
    	if self.emitter.with_actions_as_comments {
    	let indentation = self.emitter.indent().repeat(2);
    	emitted.push_str(format!("{}// {}\n", indentation, action.title).as_str());
    	}
    	Ok(emitted)
    	}

• Add tests for both of the above points.
• Add it to the docs in https://github.com/alexfertel/bulloak/blob/main/src/lib.rs and https://github.com/alexfertel/bulloak/blob/main/README.md

If I run bulloak check from the root of the project, like this:

$ bulloak check ./**/*.tree

And there is a mismatch, I get an error log like this:

Codegen not found: Couldn't find a corresponding element for "test_WhenXIsTrue" in the solidity file.

This is vague. It tells me neither the file in nor the line number where the mismatch was found.

For maximum clarity, Bulloak should print this information to the terminal.

From #7:

When naming the function tests, it should be "RevertWhen" instead of "RevertsWhen" (without the "s"). All functions should include underscores between the word "test" and "RevertWhen," or "test" and the function name. You can have a look at point 6 in the Foundry book's best practices.

With the current .tree syntax, there is no way to select a name for the test contract. The solution, for now, is to map the name of the root node's filename to a camelcase string and append Test to it, which can be weird in some cases.

It would be nice to have proper syntax that allowed such a thing. I propose the following:

withdrawMax.t.sol::WithdrawMaxTest
                 ^^^^^^^^^^^^^^^^^

That is, adding a :: after the filename and the contract name after.

The implementation would look like this:

• Add a new terminal: DoubleColon in tokenizer.rs.
• Scan it, the same way we do with // (comments), but scanning the word after the double colon.
• Add a production for DoubleColon in parser.rs.
• Add a contract_name field to Ast::Root.
• Use contract_name when emitting the contract.

Currently when bulloak check runs without an error, it just prints:

bulloak check [input file path]

adding a success message would make it more clear there were no errors. As a new user, it's unclear if the action actually ran.

The code generated by bulloak scaffold contains only 2 spaces.

But the community standard is to use 4 spaces for indenting Solidity code. Docs reference:

https://docs.soliditylang.org/en/v0.8.21/style-guide.html#indentation

Given an example:

Foo
└── It can’t do X.

bulloak scaffold panics.

Notice, ’ is used instead of '.

Given the following tree:

DynamicIds_createMintId
└── it hashes the payload, zeros last 16 bytes and returns a number

Actual result: bulloak scaffold panics:

thread 'main' panicked at /home/john/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bulloak-0.6.3/src/scaffold/mod.rs:119:38:
should format the emitted solidity code: Fmt(Error)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Expected result: bulloak removes the , character(s) and generates test_HashesThePayloadZerosLast16BytesAndReturnsANumber test.

bulloak 0.6.4