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

Rename integrity_test to post_runtime_check #1604

Closed
wants to merge 10 commits into from

Conversation

wirednkod
Copy link
Contributor

@wirednkod wirednkod commented Sep 17, 2023

As noted in paritytech/substrate#10174, this hook has a very opaque name. Based on @kianenigma 's feedback, it has never been used as much as it should have been and there are experienced substrate developers who don't know it exists, or can't really tell what it is from the name. As a result there is a need to rename this to on_construct_runtime() post_runtime_check to be more aligned with the rest of the hooks.

Closes #258

@wirednkod wirednkod added T1-FRAME This PR/Issue is related to core FRAME, the framework. I4-refactor Code needs refactoring. labels Sep 17, 2023
@bkchr
Copy link
Member

bkchr commented Sep 17, 2023

on_construct_runtime doesn't make more sense and also is not a great name.

@wirednkod
Copy link
Contributor Author

wirednkod commented Sep 17, 2023

on_construct_runtime doesn't make more sense and also is not a great name.

Naming is hard; Maybe construct_runtime_check? Your opinion or maybe a better name will matter more, since you are much more better aware on what would describe it better;

@kianenigma
Copy link
Contributor

construct_runtime_test seems like the only hope at making it a somewhat intuitive name? it is a #[test] that is created upon construct_runtime, and is populated by pieces of code that you put in this hook.

@gupnik I wonder if you have ideas such that you can improve this, or make it more explicit in #1378?

@gupnik
Copy link
Contributor

gupnik commented Sep 18, 2023

construct_runtime_test seems like the only hope at making it a somewhat intuitive name? it is a #[test] that is created upon construct_runtime, and is populated by pieces of code that you put in this hook.

@gupnik I wonder if you have ideas such that you can improve this, or make it more explicit in #1378?

For the name, I would propose post_runtime_check :)

To make it explicit after #1378, I can think of a syntax like so:

#[frame_support::construct_runtime_v2]
mod runtime {
    #[frame::pallets]
    pub struct AllPallets { ... }

     #[cfg(test)]
     pub fn post_runtime_check() {
            AllPallets::post_runtime_check();
     }
}

@wirednkod wirednkod changed the title Rename integrity_test to on_construct_runtime Rename integrity_test to post_runtime_check Sep 18, 2023
@wirednkod wirednkod marked this pull request as ready for review September 19, 2023 10:49
@wirednkod wirednkod requested review from a team September 19, 2023 10:49
@wirednkod wirednkod requested a review from a team September 19, 2023 10:49
@wirednkod wirednkod requested a review from a team as a code owner September 19, 2023 10:49
@paritytech-ci paritytech-ci requested a review from a team September 19, 2023 10:50
@svyatonik
Copy link
Contributor

I think there's some confusion with bridges here - we have our own bridge integrity tests, which have nothing common with try-runtime and/or its hooks. They are mostly about some static assertions and/or checking constants/weights, related to bridges. I'd revert all changes to bridges subtree in this PR. If you want to rename our tests too, let's do that in a separate PR, though imo our name fits our usage :)

@bkchr
Copy link
Member

bkchr commented Sep 25, 2023

They are mostly about some static assertions and/or checking constants/weights, related to bridges.

This was actually the initial idea of integrity_test and why I added it :P Checking the "integrity of the pallet in the runtime".

For the name, I would propose post_runtime_check :)

This is still more confusing than integrity_test. When is post_runtime? This doesn't give any context to the user...

@kianenigma
Copy link
Contributor

maybe this was not a great idea then and no matter what we call it, it would be confusing to some 🤷

@bkchr
Copy link
Member

bkchr commented Oct 5, 2023

So let's close this and the issue @kianenigma?

@wirednkod
Copy link
Contributor Author

As discussed with @kianenigma this can close. Thank you @bkchr

@wirednkod wirednkod closed this Oct 11, 2023
@ggwpez ggwpez mentioned this pull request Feb 5, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* clipy

* revert some fixes that newest clippy reports as issues, but older does not
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename IntegrityTest
5 participants