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.

Sign up for GitHub

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

Jump to bottom

tests(storage-provider-pallet): add some additional tests #128

Merged
merged 11 commits into from
Jul 15, 2024
Merged

tests(storage-provider-pallet): add some additional tests #128

merged 11 commits into from
Jul 15, 2024

Conversation

cernicc
Copy link
Member

@cernicc cernicc commented Jul 11, 2024 •
edited
Loading

Description

Added some additional tests to the storage provider pallet

Important points for reviewers

Added some todos on which we can discuss if you don't agree with the todos. There is also one test commented out. I couldn't make it work. Please take a look :D

Checklist

  • Additional tasks created
    - Refactor SectorNumber in primitives #129
  • Are there important points that reviewers should know?
  • Make sure that you described what this change does.
  • If there are follow-ups, have you created issues for them?
  • Have you tested this solution?

@cernicc cernicc self-assigned this Jul 11, 2024
@cernicc cernicc linked an issue Jul 11, 2024 that may be closed by this pull request
SP Pallet: Failure tests #122
Closed
jmg-duarte
jmg-duarte requested changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

See the attached comment

pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
th7nder
th7nder requested changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@th7nder th7nder left a comment

Choose a reason for hiding this comment

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

that's a lot of tests! thank you mate :D
btw. if you can show the coverage before and the coverage after and add it to the description, it'd be great.

pallets/storage-provider/src/lib.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/mock.rs Outdated Show resolved Hide resolved
primitives/proofs/src/types.rs Show resolved Hide resolved
pallets/storage-provider/src/mock.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
jmg-duarte
jmg-duarte requested changes Jul 12, 2024
View reviewed changes
pallets/storage-provider/src/lib.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/test.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/mock.rs Outdated Show resolved Hide resolved
th7nder
th7nder previously approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@th7nder th7nder left a comment

Choose a reason for hiding this comment

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

i'd say it's ready to go, up to you whether to implement those suggestions.
thanks!

pallets/storage-provider/src/tests/pre_commit_sector.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/tests/pre_commit_sector.rs Outdated Show resolved Hide resolved
jmg-duarte
jmg-duarte requested changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

Small changes, more on the API side.

Great job overall, tests looking so much cleaner now! 🔥

pallets/storage-provider/src/test.rs Show resolved Hide resolved
pallets/storage-provider/src/tests/mod.rs Outdated Show resolved Hide resolved
pallets/storage-provider/src/tests/pre_commit_sector.rs Outdated Show resolved Hide resolved
primitives/proofs/src/traits.rs Outdated Show resolved Hide resolved
@th7nder th7nder mentioned this pull request Jul 12, 2024
Closed
@cernicc cernicc mentioned this pull request Jul 15, 2024
Closed
th7nder
th7nder previously approved these changes Jul 15, 2024
View reviewed changes
@cernicc cernicc mentioned this pull request Jul 15, 2024
Closed
@cernicc cernicc requested review from jmg-duarte and th7nder July 15, 2024 09:07
@cernicc cernicc added the ready for review Review is needed label Jul 15, 2024
th7nder
th7nder previously approved these changes Jul 15, 2024
View reviewed changes
@cernicc cernicc added ready for review Review is needed and removed ready for review Review is needed labels Jul 15, 2024
jmg-duarte
jmg-duarte approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

LGTM

@jmg-duarte jmg-duarte merged commit ee698a0 into develop Jul 15, 2024
3 checks passed
@jmg-duarte jmg-duarte deleted the feat/122/sp-pallet-failure-tests branch July 15, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmg-duarte jmg-duarte jmg-duarte approved these changes

@th7nder th7nder th7nder approved these changes

@aidan46 aidan46 Awaiting requested review from aidan46

Assignees

@cernicc cernicc

Labels
ready for review Review is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SP Pallet: Failure tests
4 participants
@cernicc @th7nder @jmg-duarte @aidan46