Expand forge test --match interface (again)

[lattejed]

From here #388 (comment)

I'm in the process of migrating a larger dapptools project to foundry. We have separate contracts for rpc based tests and local tests grouped by directory. --match from dapptools allows for specifying paths / sub directories to tests which works well in our case.

The only way to do this with forge atm would be to have an external script which generates a custom regex string and supply that to --match-contract.

To streamline this I would either extend --match-contract to account for the path to the contract file or add a new --match-dir / --match-file flag.

@gakonst I'd suggest this adds

--match-path
--no-match-path

to conform to the other match flags and because there's not a 1:1 between source paths and contract names.

@mattsse was looking at gakonst/ethers-rs#802 (great work btw, looks a lot cleaner) and it looks like here and here the compiler is now returning the artifact path. Is that now a relative path (matching the relative source path) or flat in the artifacts dir?

For #392 I understood the goal was to match dapptools which would require keying on / displaying the relative source path (e.g., src/test/Greeter.t.sol:Greet)

Specific to this PR we do need the (relative) source paths to filter against. Ideally into_artifacts would return this.

It looks like you're almost done, I don't know if you want to add this in (if necessary) if not I'm happy to do it once you're done.

Also, @gakonst @mattsse I'm wondering if it's worthwhile to have the compiler skip compiling filtered-out sources? If a user is filtering out a large folder they would probably expect both testing and compilation to be skipped right?

If that makes sense, Project (assuming the name hasn't changed) could take a filter and filter source files or sth equivalent.

[mattsse]

Thanks for this write up!

rn we're storing the paths to the artifact files relative to the output dir in the cache, but the keys (the file path) absolute, because that makes it consistent, because it's not guaranteed that all solidity source files are contained inside the project's dir (remappings, relative imports).

I've added a couple of helper functions that strip/join with a base path (the project root) for example, like this https://github.com/gakonst/ethers-rs/blob/072bbb514721df67ca5359682dc081434aa632fc/ethers-solc/src/compile/output.rs#L85
Can you take a look?

into_artifacts currently returns <artifacts file.json:Contract name> like Greeter.json:Greeter. Ideally this should be abstracted over a ArtifactId {path, name, source, version} type.

we can't use src/test/Greeter.t.sol:Greet because we now support multiple versions of a compiled contract, into_artifacts would then return <artifacts file<solc version>.json:Contract name> which makes it unique.

Also, @gakonst @mattsse I'm wondering if it's worthwhile to have the compiler skip compiling filtered-out sources? If a user is filtering out a large folder they would probably expect both testing and compilation to be skipped right?

I think this would be possible now that we have a graph so we only (re)compile the contract and it's imports in question.

[lattejed]

Can you take a look?

Yeah, I'll take a look, thanks.

because we now support multiple versions of a compiled contract

Good point, my thought was these would likely be non-test files (the versioned files) but it's better to not make that assumption.

Ideally this should be abstracted over a ArtifactId {path, name, source, version} type.

Not sure what your eta is for the rewrite but I'll draft this over your branch? Ditto filtering.

I was thinking it's probably possible for the graph to point to deps that are in excluded dirs, so they'd have to get picked up anyway, but pruning opportunistically and then filtering the output would probably cover all cases, while giving a speedup where a large % of contracts are being filtered out.

[gakonst]

Agree on pruning unneeded contracts. In general, we should imo only be compiling test contracts at the edge of the graph, and not explicitly compiling imported contracts as they're not really being used. This will yield significant compilation speedup.

Right now we check for test contracts using the ABI which obviously requires having compiled them, may make sense to check the raw source for occurrences of the "function test" string to filter without needing to compile.

The only time we need to compile all is when doing forge build I think.

[lattejed]

@gakonst @mattsse I'm going to separate pre-compile filtering into a separate PR, so I've left that out.

I think it's worth discussing artifact naming more:

In another issue, we had decided that it would be best to match dapptools style output for naming artifacts:

<rel src path>:<src file>.sol:<contract name>

@mattsse pointed out that we're (maybe) compiling multiple versions of contracts, so it no longer makes sense to treat source paths as unique anymore. That's how I understood what you said at least.

I'm not sure that guarantees uniqueness however, since it's possible to compile <{A,B}.sol>:<C> without issue. Duplicate names across different source files appear to be valid.

For keying it seems we would need a combo of relative source path + artifact version to ensure uniqueness.

For display however, I'd suggest sth looser as:

1. Versioned artifacts are likely to be in deps vs test files, so I'm not sure we'll encounter multiple version of the same test file
2. dapptools style naming seems to be intuitive and avoids some dupe issues

I'd suggest this combo:

1. Use src paths + version internally to ensure uniqueness
2. Use dapptools style naming for the display
3. Add artifact paths to the display for verbosity >= 1 so users can verify if there's any confusion

Maybe I'm overlooking sth here, what do you gents think?

[mattsse]

so far so good,
smol nits

[lattejed]

NB: I've bumped semver to 1.0.5 as that's required to develop locally with ethers-rs

[lattejed]

@gakonst @mattsse @brockelmore

I'm integrating the new ArtifactId with foundry_utils::link / foundry_utils::recurse_link and was going to add some tests for these new linking routines.

I noticed this: #586 (review)

I'm also unable to put together a test project that will return anything from CompactContractBytecode::all_link_references - do you guys know what's required to do so?

I'm trying to build a passing test case for the linking steps.

[brockelmore]

https://github.com/gakonst/foundry/blob/master/cli/testdata/run_test_lib_linking.sol < see here for an example of constructing a project that needs linking

[lattejed]

Thanks @brockelmore 👍

[lattejed]

@mattsse @gakonst

I'm putting together a test for the linking that MultiContractRunner / foundry_utils::link does and making it produce multiple versions of libs that require linking.

I'm running into keying issues since we're producing versioned artifacts and [Compact]ContractBytecode::all_link_references keys off of source files. Now, this is just how the solidity compiler works, populating link_references with lib refs keyed off of source file paths/names.

Sanity check: Am I overlooking something or do we need ether-solc to produce link_references that have been re-mapped to versioned artifacts as opposed to source files?

[mattsse]

@mattsse @gakonst

I'm putting together a test for the linking that MultiContractRunner / foundry_utils::link does and making it produce multiple versions of libs that require linking.

I'm running into keying issues since we're producing versioned artifacts and [Compact]ContractBytecode::all_link_references keys off of source files. Now, this is just how the solidity compiler works, populating link_references with lib refs keyed off of source file paths/names.

I think I can follow. the references of course refer to those contracts compiled with the same version. Reading this makes me think that we need a more sophisticated type that ProjectCompileOutput::into_artifacts returns.
Perhaps this type (ProjectArtifacts or something) could work as some kind of index that

• a) returns all artifacts (same as existing into_artifacts)
• b) keeps proper indexes of (ArtifactId -> Version), (Version -> Set(ArtifactId)). This can be simplified a bit once we have a draft. I guess this index should be a separate type so you can do something like split(self) -> (Artifacts, index), basically consuming into_artifacts and also returning the index.

I hope that made sense @lattejed ?

[lattejed]

@mattsse sorry if I wasn't clear. I'll give a contrived example. We have:

TestContract1 // 0.8.10
TestContract2 // 0.8.11

^ both are identical, just diff versions

Both link to lib TestLib

We get compiler output:

TestContract1
TestContract2
TestLib.0.8.10
TestLib.0.8.11

For artifact output, we get versioned names TestLib.0.8.10 /
TestLib.0.8.11.

For linked references, we get TestLib / TestLib.

Now, this is correct, since solc references source files, not artifacts.

I assume we want to reference the versioned output since it may differ.

We're already breaking from the solc convention of using source names, so should we be remapping linking references from source names -> versioned artifact names?

[lattejed]

@mattsse yeah, I do understand what you're suggesting.

My question, is it more correct to:

1. Return enough info from into_artifacts to map link references -> versioned artifacts
2. Update all_link_references to return versioned artifact names
3. Updated link_references so ethers-rs is using versioned artifact names internally (breaking from the solc convention)
4. Maybe a compromise, have a second version of all_link_references that does what #\2 does - no breaking changes internally or externally

Embedded in that - is using a combo of source names and versioned artifacts names internally in ethers-rs potentially causing issues that haven't been uncovered yet?

Again, solc uses source names for links but solc can make assumptions that ethers-rs is breaking in order to support multiple simultaneous solc versions.

I may be overlooking sth or my OCD may be creating issues where there are none -- just want to clarify this so I don't ship a new source of bugs.

[mattsse]

There'll be some Artifact type changes here gakonst/ethers-rs#907 and here #762
so this would need a rebase.

We're already breaking from the solc convention of using source names, so should we be remapping linking references from source names -> versioned artifact names?

1. Return enough info from into_artifacts to map link references -> versioned artifacts

yeah, this is what I was thinking here #662 (comment), right?

2. Update all_link_references to return versioned artifact names

Good idea, we want everything to be identifiable, either ArtifactId or something like ReferenceId, maybe they're similar?

4. Maybe a compromise, have a second version of all_link_references that does what #\2 does - no breaking changes internally or externally

sgtm

[gakonst]

@lattejed @mattsse tests seem to pass now, is this good to go?

[mattsse]

@lattejed @mattsse tests seem to pass now, is this good to go?

lgtm, @lattejed any objections? the ethers-rs version bug is fixed here gakonst/ethers-rs#930