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

contracts: remove op-nft #12950

Merged
merged 3 commits into from
Nov 20, 2024
Merged

contracts: remove op-nft #12950

merged 3 commits into from
Nov 20, 2024

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Nov 18, 2024

Description

Remove the op-nft package from the contracts package. None of these
contracts are maintained and can be moved to another repo if necessary.
The purpose of this commit is to reduce compilation time. Less files
to compile means less compilation time.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Remove the `op-nft` package from the contracts package. None of these
contracts are maintained and can be moved to another repo if necessary.
The purpose of this commit is to reduce compilation time. Less files
to compile means less compilation time.
@tynes tynes requested a review from a team as a code owner November 18, 2024 16:58
@tynes tynes requested a review from blmalone November 18, 2024 16:58
Copy link
Contributor

@blmalone blmalone left a comment

Choose a reason for hiding this comment

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

nice, lgtm!

@tynes
Copy link
Contributor Author

tynes commented Nov 19, 2024

Hey @smartcontracts, looks like we hit an edge case in the bash script for the heavy fuzz CI job

#!/bin/bash -eo pipefail
TEST_FILES=$(git diff origin/develop...HEAD --name-only -- './test/**/*.t.sol' | sed 's|packages/contracts-bedrock/||')
TEST_FILES=$(echo "$TEST_FILES" | circleci tests split --split-by=timings)
TEST_FILES=$(echo "$TEST_FILES" | sed 's|^test/||')
MATCH_PATH="./test/{$(echo "$TEST_FILES" | paste -sd "," -)}"
forge test --match-path "$MATCH_PATH"

Error reading historical timing data: file does not exist
Requested weighting by historical based timing, but they are not present. Falling back to weighting by name.
No tests match the provided pattern:
        match-path: `./test/{periphery/op-nft/AttestationStation.t.sol,periphery/op-nft/Optimist.t.sol,periphery/op-nft/OptimistAllowlist.t.sol,periphery/op-nft/OptimistInviter.t.sol}`
Error: 
No tests to run

Exited with code exit status 1

When a file is deleted, we need to be able to handle that case. Perhaps filter out before passing to foundry?

@smartcontracts
Copy link
Contributor

Hey @smartcontracts, looks like we hit an edge case in the bash script for the heavy fuzz CI job

#!/bin/bash -eo pipefail
TEST_FILES=$(git diff origin/develop...HEAD --name-only -- './test/**/*.t.sol' | sed 's|packages/contracts-bedrock/||')
TEST_FILES=$(echo "$TEST_FILES" | circleci tests split --split-by=timings)
TEST_FILES=$(echo "$TEST_FILES" | sed 's|^test/||')
MATCH_PATH="./test/{$(echo "$TEST_FILES" | paste -sd "," -)}"
forge test --match-path "$MATCH_PATH"

Error reading historical timing data: file does not exist
Requested weighting by historical based timing, but they are not present. Falling back to weighting by name.
No tests match the provided pattern:
        match-path: `./test/{periphery/op-nft/AttestationStation.t.sol,periphery/op-nft/Optimist.t.sol,periphery/op-nft/OptimistAllowlist.t.sol,periphery/op-nft/OptimistInviter.t.sol}`
Error: 
No tests to run

Exited with code exit status 1

When a file is deleted, we need to be able to handle that case. Perhaps filter out before passing to foundry?

Good catch, thanks, I can open a PR tomorrow to filter out removed files

Updates the heavy fuzz filter for CI to only check for added or
modified contracts, now excluding deleted or moved contracts.
@smartcontracts smartcontracts requested a review from a team as a code owner November 20, 2024 08:47
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.77%. Comparing base (bc6f8de) to head (4d04c3a).
Report is 35 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12950      +/-   ##
===========================================
- Coverage    68.78%   66.77%   -2.02%     
===========================================
  Files           56       56              
  Lines         4665     4665              
===========================================
- Hits          3209     3115      -94     
- Misses        1274     1378     +104     
+ Partials       182      172      -10     
Flag Coverage Δ
cannon-go-tests-32 61.85% <ø> (-2.02%) ⬇️
cannon-go-tests-64 56.72% <ø> (-1.66%) ⬇️

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

see 8 files with indirect coverage changes

---- 🚨 Try these New Features:

@smartcontracts smartcontracts added this pull request to the merge queue Nov 20, 2024
Merged via the queue into develop with commit 39c3b5b Nov 20, 2024
52 checks passed
@smartcontracts smartcontracts deleted the contracts/delete-dead-op-nft branch November 20, 2024 10:55
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.

None yet

3 participants