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

test(refactor): core tests to use Bulloak #1022

Merged
merged 15 commits into from
Aug 28, 2024
Merged

test(refactor): core tests to use Bulloak #1022

merged 15 commits into from
Aug 28, 2024

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Aug 20, 2024

Partially closes #799

Tasks

  • Refactor core tests to comply to bulloak check
  • Rewrite core tests to conform to BTT specs

@smol-ninja smol-ninja marked this pull request as draft August 20, 2024 18:18
@smol-ninja smol-ninja marked this pull request as ready for review August 21, 2024 22:24
@smol-ninja smol-ninja force-pushed the test/bulloak branch 5 times, most recently from 6e61f86 to 8dc8971 Compare August 22, 2024 11:50
@smol-ninja smol-ninja changed the title test(refactor): refactor tests using Bulloak test(refactor): refactor core tests using Bulloak Aug 25, 2024
@smol-ninja
Copy link
Member Author

This PR is now ready for review. I will create a separate PR for periphery so that it will be easier to manage conflicts with #1024.

@smol-ninja smol-ninja changed the title test(refactor): refactor core tests using Bulloak test(refactor): core tests using Bulloak Aug 25, 2024
@smol-ninja smol-ninja changed the title test(refactor): core tests using Bulloak test(refactor): core tests to use Bulloak Aug 25, 2024
@andreivladbrg
Copy link
Member

andreivladbrg commented Aug 27, 2024

Before leaving a full review, I can see that you've used in tree branches the "is" verb.

e.g.

├── when sender is zero address
│ └── it should revert
└── when sender is not zero address
├── when recipient is zero address
│ └── it should revert
└── when recipient is not zero address
├── when deposit amount is zero
│ └── it should revert
└── when deposit amount is not zero
├── when start time is zero
│ └── it should revert
└── when start time is not zero
├── when segment count is zero
│ └── it should revert
└── when segment count is not zero

On flow repo, we have discussed about it and we have reached a common ground that it is better to not use it.

Did you change your mind? Or is it good to be changed?

@smol-ninja
Copy link
Member Author

Agree. "is" doesn't need to be there. You can go ahead and change it in this case.

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Shub.

I must say, the .tree files now look muuuch better and feel fresher. Having long branches, it was super mentally exhausting when choosing one. It’s like going from the polluted city center to the fresh air in the mountains, haha.


re does not and not, i guess you used the shorter form for consistency, right?

@smol-ninja
Copy link
Member Author

It’s like going from the polluted city center to the fresh air in the mountains

Indeed. Or more like cleaning the polluted city 😬

@PaulRBerg
Copy link
Member

Unfortunately, I am unable to review this at the moment. I will review all recent work in the Lockup repo when I perform a massive audit before the next launch.

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-05-08 at 2 57 22 PM

@smol-ninja smol-ninja merged commit 8558584 into staging Aug 28, 2024
9 checks passed
@smol-ninja smol-ninja deleted the test/bulloak branch August 28, 2024 15:25
andreivladbrg added a commit that referenced this pull request Sep 11, 2024
* ci: bulloak check

* test(refactor): refactor lockup integration tests using bulloak

* test(refactor): refactor lockup dynamic tests using bulloak

* test(refactor): refactor lockup linear tests using bulloak

* test(refactor): refactor lockup tranched tests using bulloak

* test(refactor): refactor nft descriptor tests using bulloak

* test(refactor): fix bugs in core tests using bulloak

* ci: update tree-path as per latest bulloak changelog

* test(refactor): polish core tests using BTT

* test(fix): bulloak check

* test(refactor): streamedAmountOf

* ci(refactor): remove newline

* test: remove "is" from test branches

* refactor: re-order checks in withdraw function

* test(tree): delete are keywords from branches

---------

Co-authored-by: andreivladbrg <andreivladbrg@gmail.com>
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