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-bedrock: visibility into flake #8072

Merged
merged 2 commits into from
Nov 7, 2023
Merged

contracts-bedrock: visibility into flake #8072

merged 2 commits into from
Nov 7, 2023

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Nov 7, 2023

Description

A flake in CI was introduced when #7928
was merged. It is probably due to a race condition when reading a file
from disk. Is there a way to have foundry only do something once for the
entire test suite? Tried moving things to the constructor instead of
setUp but that did not work. Ideally we do not need to read the file
from disk for each contract deployed, this adds a lot of overhead.
A solution around this is to refactor the way that the deploy script
works or to add in the env var that will skip the check that sometimes
fails.

A flake in CI was introduced when #7928
was merged. It is probably due to a race condition when reading a file
from disk. Is there a way to have foundry only do something once for the
entire test suite? Tried moving things to the constructor instead of
`setUp` but that did not work. Ideally we do not need to read the file
from disk for each contract deployed, this adds a lot of overhead.
A solution around this is to refactor the way that the deploy script
works or to add in the env var that will skip the check that sometimes
fails.
@tynes tynes requested a review from a team as a code owner November 7, 2023 12:27
@tynes tynes requested a review from smartcontracts November 7, 2023 12:28
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #8072 (98e7cff) into develop (5ebbc08) will decrease coverage by 3.98%.
Report is 6 commits behind head on develop.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #8072      +/-   ##
===========================================
- Coverage    46.07%   42.10%   -3.98%     
===========================================
  Files          170      122      -48     
  Lines         7121     4741    -2380     
  Branches      1138      987     -151     
===========================================
- Hits          3281     1996    -1285     
+ Misses        3713     2672    -1041     
+ Partials       127       73      -54     
Flag Coverage Δ
cannon-go-tests ?
chain-mon-tests 26.95% <ø> (ø)
common-ts-tests ?
contracts-bedrock-tests 40.30% <ø> (ø)
contracts-ts-tests 100.00% <ø> (ø)
core-utils-tests ?
sdk-next-tests 41.92% <ø> (ø)
sdk-tests 41.92% <ø> (ø)

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

Files Coverage Δ
packages/contracts-bedrock/scripts/Deployer.sol 0.00% <ø> (ø)

... and 50 files with indirect coverage changes

@tynes tynes merged commit cd156ff into develop Nov 7, 2023
@tynes tynes deleted the ctb/better-logging branch November 7, 2023 14:10
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