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

Feat/add bulker tests e2e #68

Merged
merged 34 commits into from
Aug 8, 2023
Merged

Feat/add bulker tests e2e #68

merged 34 commits into from
Aug 8, 2023

Conversation

julien-devatom
Copy link
Contributor

@julien-devatom julien-devatom commented Jul 24, 2023

Description of change

Closes: #60

Pull-Request Checklist

  • Code is up-to-date with the main branch
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions outlined in the conventional commit spec

@julien-devatom julien-devatom self-assigned this Jul 24, 2023
@ghivert ghivert self-assigned this Jul 26, 2023
@ghivert ghivert force-pushed the feat/add-bulker-tests-e2e branch from 452fb30 to 4f1b164 Compare July 26, 2023 08:42
@ghivert ghivert requested a review from oumar-fall July 26, 2023 09:42
@ghivert ghivert marked this pull request as ready for review July 26, 2023 09:42
@ghivert ghivert force-pushed the feat/add-bulker-tests-e2e branch 3 times, most recently from bc01648 to 12bff66 Compare July 26, 2023 10:24
Copy link
Contributor Author

@julien-devatom julien-devatom left a comment

Choose a reason for hiding this comment

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

I cannot approve but lgtm

Copy link
Contributor

@oumar-fall oumar-fall left a comment

Choose a reason for hiding this comment

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

in summary:

  • variable names shouldn't be shortened (wAllowance => wethAllowance)
  • test suites names should be clearer
  • please review this pr and rebase
  • test assertion error should always contain expected vs received value --> expected [...] to be ${expectedValue}, received ${receivedValue}
  • no need to do refreshAll + refetchData
  • the mechanism around the approve function is weird (why do we need to revoke the approval since we're reverting to a snapshot between every tests)? If really needed I would rather approve everything in the beforeEach function and revoke everything in the afterEach

tests/helpers/bn.ts Outdated Show resolved Hide resolved
src/utils/signatures/withSigner.ts Outdated Show resolved Hide resolved
src/utils/signatures/withSigner.ts Outdated Show resolved Hide resolved
src/txHandler/Bulker.TxHandler.ts Outdated Show resolved Hide resolved
src/txHandler/Bulker.TxHandler.ts Outdated Show resolved Hide resolved
tests/e2e/bulker.test.ts Outdated Show resolved Hide resolved
tests/e2e/bulker.test.ts Outdated Show resolved Hide resolved
tests/e2e/bulker.test.ts Outdated Show resolved Hide resolved
tests/e2e/bulker.test.ts Outdated Show resolved Hide resolved
tests/e2e/bulker.test.ts Outdated Show resolved Hide resolved
@ghivert ghivert force-pushed the feat/add-bulker-tests-e2e branch from 12bff66 to 24cddf8 Compare July 27, 2023 11:27
@ghivert ghivert requested a review from oumar-fall July 27, 2023 11:28
@ghivert
Copy link
Contributor

ghivert commented Jul 27, 2023

Most of reviews have been applied.

On the approve function (renamed to approveBulkerOrPermit2), it is unfortunately required, because I'm not allowing the same contract on each run. Sometimes it's the bulker, sometimes it's Permit2. So we can't put it in beforeEach.

@oumar-fall On another note, I'm not sure the renaming of test suite really makes a difference, but I'm not disturbed by the change neither. I feel like it's a little bit micro-managing the codebase while the tests names are explicit and that we don't have precise conventions on this. I think it would be good to have conventions then, to avoid renaming everything everytime. (I'm not even 100% sure the renaming will suit you based on your comment, to be honest. 😳 I think it can quickly lead to anyone having its own naming convention by habit, and renaming everything many times, for no real improvements of the codebase.) I'd be happy to have your opinion and a little bit more of what you think about that, what was unclear in the naming, etc.

On the naming variables, I fully agree though, namings should be consistent, that was a good point.

@ghivert ghivert force-pushed the feat/add-bulker-tests-e2e branch from 24cddf8 to f6567ec Compare July 27, 2023 12:36
src/txHandler/Bulker.TxHandler.ts Outdated Show resolved Hide resolved
tests/helpers/bn.ts Outdated Show resolved Hide resolved
tests/e2e/bulker.test.ts Show resolved Hide resolved
Copy link
Contributor

@oumar-fall oumar-fall left a comment

Choose a reason for hiding this comment

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

format big numbers in messages

tests/e2e/bulker.test.ts Show resolved Hide resolved
oumar-fall
oumar-fall previously approved these changes Aug 8, 2023
@oumar-fall oumar-fall dismissed their stale review August 8, 2023 13:56

missing fix

@ghivert ghivert requested a review from oumar-fall August 8, 2023 14:35
ghivert added 23 commits August 8, 2023 16:36
Signed-off-by: Guillaume Hivert <[email protected]>
Signed-off-by: Guillaume Hivert <[email protected]>
@ghivert ghivert force-pushed the feat/add-bulker-tests-e2e branch from b1b280f to 9016a89 Compare August 8, 2023 14:36
@oumar-fall oumar-fall merged commit 3ec9ed3 into main Aug 8, 2023
@oumar-fall oumar-fall deleted the feat/add-bulker-tests-e2e branch August 8, 2023 15:23
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add e2e tests for bulker logic
3 participants