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

Refactor staking test modules into their own files #3235

Closed

Conversation

vicenterocha
Copy link

@vicenterocha vicenterocha commented Feb 6, 2024

This PR refactors substrate staking tests, as detailed in #3178.

The idea is to do this in 2 PRs. On this one:

  1. Move all mod tests from tests.rs into their own test file under a new tests/ folder
  2. Move all non mod tests from tests.rs into tests/mod.rs

Even we this changes the tests/mod.rs is still quiet big (6k LOC). Next PR should address it by extrating tests/mods.rs into further submodules.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Feb 6, 2024

User @vicenterocha, please sign the CLA here.

@gpestana gpestana self-requested a review February 6, 2024 21:43
@gpestana gpestana added T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet. T10-tests This PR/Issue is related to tests. labels Feb 6, 2024
@vicenterocha vicenterocha reopened this Feb 6, 2024
@gpestana
Copy link
Contributor

gpestana commented Feb 6, 2024

@vicenterocha thanks for the PR! One quick comment, we try to avoid rewriting history/force push on intermediate commits on open PRs -- especially on PRs ready to review, which is not the case here but it's still better to not squash int commits in general. The commits will be squashed before merging so no problem with adding a bunch of commits to the PR.

@gpestana gpestana added the R0-silent Changes should not be mentioned in any release notes label Feb 7, 2024
@vicenterocha vicenterocha marked this pull request as ready for review February 8, 2024 21:58
Copy link
Contributor

@gpestana gpestana left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

regarding the failing jobs, there was a recent fix to polkavm that should fix the build-runtimes-polkavm job -- could you merge the most recent master into your branch?

@Ank4n Ank4n self-requested a review February 26, 2024 10:58
@Ank4n
Copy link
Contributor

Ank4n commented Feb 28, 2024

@vicenterocha thanks for the contribution. Quick question, when I run these tests (cargo test -p pallet-staking) I see one less test being ran than the previous count (Old: 156, New: 155). Was there a test you removed or?

@Ank4n Ank4n requested review from kianenigma and rossbulat and removed request for kianenigma February 28, 2024 20:32
@vicenterocha
Copy link
Author

vicenterocha commented Mar 2, 2024

@Ank4n thanks for having a look. Running tests on this branch actually gives me 156, whereas on master it gives me 155.

refactor-staking-tests:
Screenshot 2024-03-02 at 22 20 49

master:
Screenshot 2024-03-02 at 22 23 03

I've compare both runs and it seems like this is the the additional test:
test mock::test_genesis_config_builds ... ok

I can provide a full test output of both master and current branch.

Not sure exactly how this test is defined though.

@Ank4n
Copy link
Contributor

Ank4n commented Mar 6, 2024

@Ank4n thanks for having a look. Running tests on this branch actually gives me 156, whereas on master it gives me 155.

refactor-staking-tests: Screenshot 2024-03-02 at 22 20 49

master: Screenshot 2024-03-02 at 22 23 03

I've compare both runs and it seems like this is the the additional test: test mock::test_genesis_config_builds ... ok

I can provide a full test output of both master and current branch.

Not sure exactly how this test is defined though.

Thanks for investigating it. The test was added couple of weeks ago. My guess is I ran the test with your branch that was not updated with master which is why I missed one. And vice-versa for you, your master might be outdated.

@vicenterocha
Copy link
Author

Running on this branch

Screenshot 2024-04-14 at 17 36 43

Running on master

image

@gpestana
Copy link
Contributor

@vicenterocha thanks a lot for helping with this work, we have a bunch of changes coming up related to the pallet-staking with the multi-block election work and I think it's better to refactor all the tests during that work. In any case, I can point you some other issues to help with if you want. Thanks :)

@gpestana gpestana closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet. T10-tests This PR/Issue is related to tests.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants