Resolution for duplicate test/contract names affecting forge test output

[stevennevins]

Demo repo: https://github.com/stevennevins/demo_behavior

Dapptools seems to handle if there are harness contracts with duplicate names and *.t.sol files with duplicate test case names, but forge will only display the result of the first case of these duplicates unless verbosity is increased. I've included a demo repo to show the behavior difference between dapptools and forge.

[gakonst]

I suspect this is happening due to the .collect() here, because it's done over Contract name, not including the file name.

[lattejed]

Looks like the core issue here, like @gakonst pointed out, is that BTreeMap is being used and test names in forge are of the format:

<filename>:<name>

so duplicate names as keys are being discarded. In dapptools they're fully qualified:

<path>/<filename>:<name>

An easy fix would be to replace BTreeMap with a Vec of tuples or similar.

It also might make sense to change it upstream here since the current naming convention discards info.

@gakonst / @mattsse how do you feel about this being changed upstream? Not sure if breaking changes will be an issue here.

[lattejed]

Here's the output from the example project if any clarity is needed:

dapptools:

Running 3 tests for src/test/Greeter.t.sol:Gm
[PASS] testOwnerCanGmOnGoodBlocks() (gas: 27568)
[PASS] testNonOwnerCannotGm() (gas: 2969)
[PASS] testOwnerCannotGmOnBadBlocks() (gas: 3686)

Running 3 tests for src/test/Greeter.t.sol:Greet
[PASS] testCannotGm() (gas: 4540)
+++ OK, passed 100 tests.
[PASS] testWorksForAllGreetings(string) (runs: 100)
[PASS] testCanSetGreeting() (gas: 28591)

Running 3 tests for src/test/Greeter_2.t.sol:Gm
[PASS] testOwnerCanGmOnGoodBlocks() (gas: 27568)
[PASS] testNonOwnerCannotGm() (gas: 2969)
[PASS] testOwnerCannotGmOnBadBlocks() (gas: 3686)

Running 3 tests for src/test/Greeter_2.t.sol:Greet
[PASS] testCannotGm() (gas: 4540)
+++ OK, passed 100 tests.
[PASS] testWorksForAllGreetings(string) (runs: 100)
[PASS] testCanSetGreeting() (gas: 28591)

forge:

Running 3 tests for Gm.json:Gm
[PASS] testNonOwnerCannotGm() (gas: 3782)
[PASS] testOwnerCanGmOnGoodBlocks() (gas: 31696)
[PASS] testOwnerCannotGmOnBadBlocks() (gas: 7771)

Running 3 tests for Greet.json:Greet
[PASS] testCanSetGreeting() (gas: 31070)
[PASS] testCannotGm() (gas: 6819)
[PASS] testWorksForAllGreetings(string) (runs: 256, μ: 71779, ~: 78350)

[lattejed]

@mattsse what do you think about changing ethers-solc to return names that are more in line with dapptools? Or, possibly keeping things as they are but storing / exposing the additional info (path, anything else that makes sense) so it can be used downstream?

Happy to do it

[mattsse]

@lattejed definitely, can you open an issue on ethers-rs with a rational/proposal.
I also like the idea of making this configurable with the upcoming config, https://github.com/gakonst/foundry/blob/f9007fa8c238ffbb1365b26460a949ead303c046/config/README.md

there are other things we should think about when it comes to naming of the artifacts: preventing artifact collisions entirely

for example
if we compile a contract twice with different solc: A(<=0.8.10) imports C(>0.4.0) and B(0.8.11) imports C(>0.4.0)

this is likely not a big deal if C is a library import, but could as well be another source contract.

we also would run in to a collision if we have file:ContractName duplicates
like

v1
|__core.sol{BasicContract}
v2
|__core.sol{BasicContract}

[lattejed]

Sounds good @mattsse I'll do that. I caught the artifact discussion elsewhere, I'll take a look including that as well.

[lattejed]

Opened an issue here for discussion gakonst/ethers-rs#791

[lattejed]

Upstream PR changes output for forge to:

Running 3 tests for src/test/Greeter.t.sol:Gm
[PASS] testNonOwnerCannotGm() (gas: 3782)
[PASS] testOwnerCanGmOnGoodBlocks() (gas: 31696)
[PASS] testOwnerCannotGmOnBadBlocks() (gas: 7771)

Running 3 tests for src/test/Greeter.t.sol:Greet
[PASS] testCanSetGreeting() (gas: 31070)
[PASS] testCannotGm() (gas: 6819)
[PASS] testWorksForAllGreetings(string) (runs: 256, μ: 76067, ~: 78410)

Running 3 tests for src/test/Greeter_2.t.sol:Gm
[PASS] testNonOwnerCannotGm() (gas: 3782)
[PASS] testOwnerCanGmOnGoodBlocks() (gas: 31696)
[PASS] testOwnerCannotGmOnBadBlocks() (gas: 7771)

Running 3 tests for src/test/Greeter_2.t.sol:Greet
[PASS] testCanSetGreeting() (gas: 31070)
[PASS] testCannotGm() (gas: 6819)
[PASS] testWorksForAllGreetings(string) (runs: 256, μ: 76067, ~: 78410)

PR still WIP