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

Tidy up and restructure of pool unit-tests #3507

Merged
merged 6 commits into from
Feb 16, 2023
Merged

Tidy up and restructure of pool unit-tests #3507

merged 6 commits into from
Feb 16, 2023

Conversation

Ben-PH
Copy link
Contributor

@Ben-PH Ben-PH commented Feb 7, 2023 •
edited
Loading

  • document all added functions
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@Ben-PH Ben-PH self-assigned this Feb 7, 2023
@Ben-PH Ben-PH added refactoring tests Issues related to testing the node labels Feb 7, 2023
@Ben-PH Ben-PH linked an issue Feb 7, 2023 that may be closed by this pull request
[follow-up #3846] Restructure pool-tests #3506
Closed
5 tasks
@Ben-PH Ben-PH marked this pull request as ready for review February 9, 2023 11:07
@Ben-PH Ben-PH requested a review from AurelienFT February 9, 2023 11:07
@Ben-PH Ben-PH marked this pull request as draft February 9, 2023 12:53
@Ben-PH Ben-PH force-pushed the pool-test-restructure branch from d928f4b to 47e1b73 Compare February 9, 2023 16:02
Ben-PH
Ben-PH commented Feb 9, 2023
View reviewed changes
massa-pool-worker/src/tests/operation_pool_tests.rs
Comment on lines +63 to +65
for i in 0..18 {
let expire_period: u64 = 40 + i;
let op = OpGenerator::default()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally this code could be handled by a helper method, as similar work is done in the other test functions. This is not so straight forward, as there are subtle differences between the tests that would make this doable, but in my view "clever for the sake of clever", and not of significant benefit in this context.

@AurelienFT
Copy link
Contributor

This PR Is still in draft what's the current state ?

@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 15, 2023

Needed to ask you about:

a) the flakey tests. They fail every now and then, but generally, the CI accounts for that with its retries. I believe it will reliably fail if there is a bug that it's checking for.
b) It was mentioned that it would be nice to have a helper method that handles the setup based on the way it's called, with an iteration count. The nuances of how things are setup differently makes this... tricky. Sure it can be done, but I'm not sure it's a great use of time, and not sure if it would make the test code more readable-maintainable anyway.

Not sure what the CI failure is about, though.

@AurelienFT
Copy link
Contributor

Thanks for precisions @Ben-PH let's review it and merge it as it is for now and iterate in the futur to avoid letting this PR die. Can you update on testnet_20 to fix the gas_calibration check and put it as ready ?

@Ben-PH
Copy link
Contributor Author

Ben-PH commented Feb 15, 2023

re-ran the failed CI test. Passed. marking as ready.

@Ben-PH Ben-PH marked this pull request as ready for review February 15, 2023 23:03
AurelienFT
AurelienFT approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@AurelienFT AurelienFT 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 really nice, we cover a lot more cases now. Nice work !

@Ben-PH Ben-PH merged commit 611fb2c into testnet_20 Feb 16, 2023
@Ben-PH Ben-PH mentioned this pull request Feb 17, 2023
Closed
@AurelienFT AurelienFT mentioned this pull request Feb 27, 2023
Merged
bors bot added a commit that referenced this pull request Mar 1, 2023
@bors @AurelienFT
@Ben-PH @modship @Eitu33 @sydhds @aoudiamoncef
Merge #3489
f01d16a 
3489: Testnet 20 r=AurelienFT a=AurelienFT

Merged in this testnet:

- #3475 
- #3549
- #3562 
- #3462 
- #3492 
- #3502 
- #3495 
- #3556 
- #3511 
- #3498 
- #3566 
- #3557 
- #3576 
- #3579 
- #3507 
- #3585 
- #3587 
- #3580 
- #3590 
- #3549 
- #3455 
- #3601 
- #3602 
- #3606 
- #3588 
- #3603 
- #3554

Co-authored-by: AurelienFT <aurelien.foucault@epitech.eu>
Co-authored-by: Ben PHL <benphawke@gmail.com>
Co-authored-by: Modship <yeskinokay@gmail.com>
Co-authored-by: Ben <benphawke@gmail.com>
Co-authored-by: Eitu33 <89928840+Eitu33@users.noreply.github.com>
Co-authored-by: Sydhds <sylvain.delhomme@gmail.com>
Co-authored-by: modship <lu@massa.net>
Co-authored-by: Moncef AOUDIA <22281426+aoudiamoncef@users.noreply.github.com>
Co-authored-by: Moncef AOUDIA <ma@massa.net>
@Ben-PH Ben-PH mentioned this pull request Mar 20, 2023
Closed
28 tasks
@AurelienFT AurelienFT deleted the pool-test-restructure branch May 29, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AurelienFT AurelienFT AurelienFT approved these changes

Assignees

@Ben-PH Ben-PH

Labels
tests Issues related to testing the node
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[follow-up #3846] Restructure pool-tests
2 participants
@Ben-PH @AurelienFT