Skip to content

increase coverage of contracts package#2209

Merged
mslipper merged 1 commit intoethereum-optimism:developfrom
tonykogias:increase-codecov-contracts-package
Mar 3, 2022
Merged

increase coverage of contracts package#2209
mslipper merged 1 commit intoethereum-optimism:developfrom
tonykogias:increase-codecov-contracts-package

Conversation

@tonykogias
Copy link
Contributor

@tonykogias tonykogias commented Feb 13, 2022

@changeset-bot
Copy link

changeset-bot bot commented Feb 13, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: 60658a2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2022 •
edited
Loading

Codecov Report

Merging #2209 (60658a2) into develop (aa726e8) will increase coverage by 3.20%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2209      +/-   ##
===========================================
+ Coverage    74.57%   77.78%   +3.20%     
===========================================
  Files           86       86              
  Lines         2962     2962              
  Branches       511      511              
===========================================
+ Hits          2209     2304      +95     
+ Misses         753      658      -95
Flag Coverage Δ
batch-submitter 62.03% <ø> (ø)
contracts 99.29% <ø> (+8.40%) ⬆️
core-utils 89.13% <ø> (ø)
data-transport-layer 49.72% <ø> (ø)
sdk 57.71% <ø> (ø)

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

Impacted Files Coverage Δ
.../contracts/L1/messaging/L1CrossDomainMessenger.sol 98.38% <0.00%> (+1.61%) ⬆️
.../contracts/L1/rollup/CanonicalTransactionChain.sol 100.00% <0.00%> (+1.90%) ⬆️
...tracts/contracts/L1/messaging/L1StandardBridge.sol 100.00% <0.00%> (+2.94%) ⬆️
.../contracts/L2/messaging/L2CrossDomainMessenger.sol 96.42% <0.00%> (+3.57%) ⬆️
...tracts/contracts/L2/messaging/L2StandardBridge.sol 100.00% <0.00%> (+4.54%) ⬆️
...racts/contracts/libraries/utils/Lib_MerkleTree.sol 100.00% <0.00%> (+6.12%) ⬆️
...ontracts/contracts/libraries/rlp/Lib_RLPWriter.sol 100.00% <0.00%> (+6.15%) ⬆️
...contracts/contracts/libraries/utils/Lib_Buffer.sol 100.00% <0.00%> (+7.14%) ⬆️
...racts/contracts/L1/rollup/StateCommitmentChain.sol 100.00% <0.00%> (+12.50%) ⬆️
.../contracts/libraries/trie/Lib_SecureMerkleTrie.sol 100.00% <0.00%> (+21.42%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa726e8...60658a2. Read the comment docs.

@tonykogias
Copy link
Contributor Author

tonykogias commented Feb 20, 2022

@smartcontracts I managed to increase the statement coverage from 90.95% to 99.18 %. As I was on it I tried to increase the branch coverage as much as possible too. I managed to raise it from 84.44% to 93.38% but I noticed that some of the requires are impossible to hit. If you have any idea on how to hit the missing statements/branches please let me know and I will try to work it out.

Also, I noticed that you are still using smock's old version. Is migrating from smock v1 to v2 something you are thinking of doing in the future? If so let me know and I can help since I got the hang of it the past few days.

@tonykogias tonykogias marked this pull request as ready for review February 20, 2022 23:03
@smartcontracts
Copy link
Contributor

smartcontracts commented Feb 27, 2022

@tonykogias this is amazing work, thank you. I have been on break this past week but I will make sure to review this first thing next week! Upgrading to smock v2 is definitely important for us in the future (smock v1 relies on some old dependencies that I'd like to remove).

smartcontracts
smartcontracts approved these changes Mar 3, 2022
View reviewed changes
Copy link
Contributor

@smartcontracts smartcontracts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really excellent work, thank you @tonykogias. Left some minor nitpicks, but otherwise looks really good.

packages/contracts/test/contracts/L1/deployment/AddressDictator.spec.ts Outdated
})

describe('returnOwnership', () => {
it('should transfer contractc ownership to finalOwner', async () => {
Copy link
Contributor

@smartcontracts smartcontracts Mar 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo here

@smartcontracts
Copy link
Contributor

smartcontracts commented Mar 3, 2022 •
edited
Loading

Let's get this combined into a few clean commits using the conventional commit format (and rebased onto develop) and we should be good to go. I don't think we need a changeset because we're only touching test code here.

@tonykogias tonykogias marked this pull request as draft March 3, 2022 21:00
@tonykogias
test(contracts): increase code coverage
60658a2 
This commit increases the statement and branch coverage of the contracts
package. More specificall the contracts that were covered are:

- AddressDictator
- ChugSplashDictator
- L1CrossDomainMessenger
- L1StandardBridge
- CanonicalTransactionChain
- ChainStorageContainer
- StateCommitmentChain
- L2CrossDomainMessenger
- L2StandardBridge
- WETH9
- Lib_OVMCode
- Lib_RLPWriter
- Lib_SecureMerkleTrie
- Lib_Buffer
- Lib_MerkleTree
@tonykogias tonykogias marked this pull request as ready for review March 3, 2022 22:13
@mslipper mslipper merged commit 5b96658 into ethereum-optimism:develop Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smartcontracts smartcontracts Awaiting requested review from smartcontracts

1 more reviewer

@mslipper mslipper mslipper approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Increase code coverage for contracts package

4 participants

@tonykogias @codecov-commenter @smartcontracts @mslipper