EVM Engineering: explore using CommonTest for everything

[smartcontracts]

Some tests currently use Bridge_Initializer, some use CommonTest, and other use Test. We are already combining Bridge_Initializer and CommonTest so it might be worth just using CommonTest everywhere.

CommonTest definitely works but may make tests take longer. We should benchmark the difference to see if it's significant. If it's not significant, then moving to CommonTest everywhere would be nice so that people don't need to think as much when writing tests, especially newer contributors. We could also add a semgrep rule against using Test directly so that this gets enforced automatically.

[AmadiMichael]

Importing CommonTest and inheriting it (instead of Test) to a test that currently inherits Test adds ~ 8 secs to compilation time on my end. Tested this with packages/contracts-bedrock/test/dispute/lib/LibClock.t.sol and packages/contracts-bedrock/test/cannon/PreimageOracle.t.sol

[tynes]

We should not be using CommonTest for testing libraries that require no contract deployments

[smartcontracts]

We should not be using CommonTest for testing libraries that require no contract deployments

Argument here is that using a single CommonTest for everything is more braindead than trying to understand if your test suite will need access to these things or not. If there's no significant impact on testing time (especially when you don't actually call super.setUp()) then having everything use CommonTest is an improvement because less brain cells go into deciding how to write your tests.

Basically, smart contract development should be as simple as possible and we should reduce the number of options down to 1 whenever we can.

[tynes]

Generally agree with the direction that you are going for but if you are a developer that cannot determine if you should use CommonTest or Test then I don't think you would be a good fit to work on this project

[smartcontracts]

I think it's mostly just that we want to keep things simple. CommonTest is the same thing as Test except for the setup function so if you don't call super.setUp then it's identical. By using CommonTest everywhere we just standardize a bit more and have less to think about. Less brain power on boilerplate when writing tests is good, because it means more brainpower goes to writing tests. It seems minor but these things add up.

[maurelian]

I agree with Mark that it doesn't make sense to test a library with an entire L1 and L2 deployed. Aggregated over hundreds of tests that will definitely slow down the tests.

IMO this is most an issue of naming, ie. CommonTest and Test are very different things that just share a similar name.

I'd propose combining CommonTest and BridgeInitializer into one contract Foo. All concrete contracts in the system should be tested against Foo.

If we organize our tests correctly, then we should be able to have semgrep tests per path, so that all tests in test/L1,test/L2, test/dispute, etc. inherit from Foo, and all others can inherit Test.

[AmadiMichael]

I'd propose combining CommonTest and BridgeInitializer into one contract Foo. All concrete contracts in the system should be tested against Foo.

This PR https://github.com/ethereum-optimism/optimism/pull/12795 merges CommonTest and BridgeInitializer 🙂

[smartcontracts]

@maurelian the key point here is that CommonTest doesn't actually deploy all of the contracts unless you explicitly tell it to - if you don't tell it to do that deployment then it behaves identically to Test. If we merged the two contracts then we basically have a single unified testing base that you can use to explicitly request deployments of other contracts if needed.

[smartcontracts]

If you don't call CommonTest.setUp() then CommonTest and Test are identical.

So my proposal here would be that we only use CommonTest and rename CommonTest.setUp to CommonTest.deployFullSystem or something similar

[maurelian]

I see, I actually think that's a reasonable idea then, esp. with renaming setUp().