Skip to content

feat(forge-std): Replaces ds-test with forge-std#214

Closed
refcell wants to merge 21 commits intotransmissions11:mainfrom
refcell:ab/forge_std
Closed

feat(forge-std): Replaces ds-test with forge-std#214
refcell wants to merge 21 commits intotransmissions11:mainfrom
refcell:ab/forge_std

Conversation

@refcell
Copy link
Contributor

@refcell refcell commented Apr 19, 2022

Description

This PR modernizes the test suite to use forge-std rather than ds-test.

Checklist

  • Ran forge snapshot?
  • Ran npm run lint?
  • Ran forge test?

@refcell refcell marked this pull request as ready for review April 20, 2022 05:35
@refcell refcell requested a review from transmissions11 April 20, 2022 05:35
Comment on lines +145 to +148
if (testBytes.length == 0) return;
endIndex = bound(endIndex, testBytes.length + 1, type(uint256).max - 1);
startIndex = bound(startIndex, testBytes.length + 1, type(uint256).max - 1);
if (startIndex > endIndex) (startIndex, endIndex) = (endIndex, startIndex);
Copy link
Owner

Choose a reason for hiding this comment

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

why can't we use the previous code again? because of the plus 1? maybe we just allow overflow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise I believe it errors with an arithmetic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this way it ensures it reverts with "OUT_OF_BOUNDS"

@transmissions11
Copy link
Owner

damn the tests run slow af with assume

@transmissions11
Copy link
Owner

wai close im sorry im slow

@refcell
Copy link
Contributor Author

refcell commented Oct 26, 2022

wai close im sorry im slow

image

@transmissions11
Copy link
Owner

lmao

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.

3 participants