Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add allow-empty overload field on Process.run #10736

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Conversation

maurelian
Copy link
Contributor

@maurelian maurelian commented Jun 4, 2024

This PR updates Process.run() to fail by default if the command does not return any output.

This causes failures in any test which uses getContractFunctionAbis, indicating that getContractFunctionAbis has been failing silently due to a zero length array of abis:

string[] memory contractNames = abi.decode(vm.parseJson(string(Process.run(command))), (string[]));
abis_ = new Abi[](contractNames.length);
for (uint256 i; i < contractNames.length; i++) {

This PR is intended to demonstrate this. A fix is also needed to getContractFunctionAbis.

Update: The changes in PR #10739 fix getContractFunctionAbis, so this change can now be applied without breaking existing functionality.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.44%. Comparing base (ab663b0) to head (3820162).

Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #10736       +/-   ##
============================================
- Coverage    60.37%   16.44%   -43.93%     
============================================
  Files           19        8       -11     
  Lines         1696      535     -1161     
  Branches        71       71               
============================================
- Hits          1024       88      -936     
+ Misses         640      447      -193     
+ Partials        32        0       -32     
Flag Coverage Δ
cannon-go-tests ?
chain-mon-tests 27.14% <ø> (ø)
sdk-tests 16.44% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 11 files with indirect coverage changes

@maurelian maurelian marked this pull request as ready for review June 5, 2024 00:50
@maurelian maurelian requested a review from a team as a code owner June 5, 2024 00:50
@maurelian maurelian requested a review from mds1 June 5, 2024 00:50
@Inphi Inphi enabled auto-merge June 5, 2024 01:01
@Inphi Inphi added this pull request to the merge queue Jun 19, 2024
Merged via the queue into develop with commit 14b4425 Jun 19, 2024
59 of 61 checks passed
@Inphi Inphi deleted the maur/allow-empty branch June 19, 2024 17:09
rdovgan pushed a commit to rdovgan/optimism that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants