alexfertel/bulloak

Implement syntax for specifying a contract name

Closed this issue · 6 comments

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.
mds1 commented

Partly related to the naming convention requirements in #19, I agree we should have a standardized approach here. For example, given this contract:

contract MyContract {
  function foo() external { ... }
  function _bar() internal { ... }
}

My first reaction was to go one spec file + one test file per method in the contract:

./test
├── MyContract.foo.t.sol
├── MyContract._bar.t.sol
├── MyContract.foo.tree
└── MyContract._bar.tree

IIUC the suggestion would mean I can do either of the following:

  1. Two .tree files as shown above, where the first line of each is MyContract.foo.t.sol::FooTest and MyContract._bar.t.sol::BarTest, which would generate (or require, per #19) the tests for those methods to be in a contract called FooTest that lives in MyContract.foo.t.sol (and similarly for _bar)
  2. Or I can have a single .tree file that looks like this:
MyContract.foo.t.sol::FooTest
└── when x (remaining items left out for brevity)

MyContract._bar.t.sol::BarTest
└── when y (remaining items left out for brevity)

and this would result in the same outputs/requirements as above.

Is that understanding correct? If so, that sounds good to me

Yes for number 1!

For number 2, bulloak currently doesn't support more than one tree per .tree file. I'd rather refrain from implementing 2 for now, since:

  • It would change quite a few things implementation-wise.
  • I'm not entirely sold on the value if brings over 1.

My first reaction was to go one spec file + one test file per method in the contract:

This is interesting though, I think this is never the case in the Sablier repo, but it makes sense to me. I wonder what @PaulRBerg's rationale was for going with whole-contract specs. (Sorry for all the pings, Paul, I hope you don't mind)

EDIT: Nvm, Paul! lol

mds1 commented

Agreed with not implementing number 2 for now, that seems reasonable.

This is interesting though, I think this is never the case in the Sablier repo, but it makes sense to me. I wonder what @PaulRBerg's rationale was for going with whole-contract specs.

I thought sablier did something similar, e.g. here it seems they have one folder for each method, and those folders each have one .tree and one .t.sol file

I thought sablier did something similar, e.g. here it seems they have one folder for each method, and those folders each have one .tree and one .t.sol file

Oh, you are right, I was misunderstanding this whole time 😳

Great ideas! I am also in favor of option 1 - it's the path of least resistance.

Though, I wonder if it's worth it to include the test file name in the tree file itself? Wouldn't just FooTest be good enough?

Sorry for all the pings, Paul, I hope you don't mind

No, not at all. Writing tests and BTT has been a labor of love on my part.

I thought sablier did something similar, e.g. here it seems they have one folder for each method, and those folders each have one .tree and one .t.sol file

Can confirm that this understanding is correct.

Having one tree file per Solidity test file makes it easier to reason about the function-under-test, and it also makes maintenance easier.

mds1 commented

Though, I wonder if it's worth it to include the test file name in the tree file itself? Wouldn't just FooTest be good enough?

Good point, I agree. The filename would convey this, so perhaps what we do is, given this contract:

contract MyContract {
  function foo() external { ... }
  function _bar() internal { ... }
}

You'd have a test folder like this:

./test
├── MyContract.foo.t.sol
├── MyContract._bar.t.sol
├── MyContract.foo.tree
└── MyContract._bar.tree

And tree files like this example for MyContract._bar.t.sol:

BarTest
└── when x (remaining items left out for brevity)

This lets people have some flexibility of their contract names, for example, since contract names can't start with an underscore, maybe I want some convention to indicate which test contracts are for internal methods, such as:

BarTest_Internal
└── when x (remaining items left out for brevity)
FooTest
└── when y (remaining items left out for brevity)