-
Notifications
You must be signed in to change notification settings - Fork 29
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
GH-552: max_block_count defaults to 100_000 #558
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
71 changes: 71 additions & 0 deletions
71
node/src/database/db_migrations/migrations/migration_9_to_10.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
use crate::database::db_migrations::db_migrator::DatabaseMigration; | ||
use crate::database::db_migrations::migrator_utils::DBMigDeclarator; | ||
|
||
#[allow(non_camel_case_types)] | ||
pub struct Migrate_9_to_10; | ||
|
||
impl DatabaseMigration for Migrate_9_to_10 { | ||
fn migrate<'a>( | ||
&self, | ||
declaration_utils: Box<dyn DBMigDeclarator + 'a>, | ||
) -> rusqlite::Result<()> { | ||
declaration_utils.execute_upon_transaction(&[ | ||
&"INSERT INTO config (name, value, encrypted) VALUES ('max_block_count', 100000, 0) ON CONFLICT(name) DO UPDATE SET value = 100000 WHERE name = 'max_block_count'" | ||
]) | ||
} | ||
|
||
fn old_version(&self) -> usize { | ||
9 | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use crate::database::db_initializer::{ | ||
DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE, | ||
}; | ||
use crate::test_utils::database_utils::{ | ||
bring_db_0_back_to_life_and_return_connection, make_external_data, retrieve_config_row, | ||
}; | ||
use masq_lib::test_utils::logging::{init_test_logging, TestLogHandler}; | ||
use masq_lib::test_utils::utils::ensure_node_home_directory_exists; | ||
use std::fs::create_dir_all; | ||
|
||
#[test] | ||
fn migration_from_9_to_10_is_properly_set() { | ||
init_test_logging(); | ||
let dir_path = ensure_node_home_directory_exists( | ||
"db_migrations", | ||
"migration_from_9_to_10_is_properly_set", | ||
); | ||
create_dir_all(&dir_path).unwrap(); | ||
let db_path = dir_path.join(DATABASE_FILE); | ||
let _ = bring_db_0_back_to_life_and_return_connection(&db_path); | ||
let subject = DbInitializerReal::default(); | ||
|
||
let result = subject.initialize_to_version( | ||
&dir_path, | ||
9, | ||
DbInitializationConfig::create_or_migrate(make_external_data()), | ||
); | ||
|
||
assert!(result.is_ok()); | ||
|
||
let result = subject.initialize_to_version( | ||
&dir_path, | ||
10, | ||
DbInitializationConfig::create_or_migrate(make_external_data()), | ||
); | ||
|
||
let connection = result.unwrap(); | ||
let (mp_value, mp_encrypted) = retrieve_config_row(connection.as_ref(), "max_block_count"); | ||
let (cs_value, cs_encrypted) = retrieve_config_row(connection.as_ref(), "schema_version"); | ||
assert_eq!(mp_value, Some(100_000u64.to_string())); | ||
assert_eq!(mp_encrypted, false); | ||
assert_eq!(cs_value, Some(10.to_string())); | ||
assert_eq!(cs_encrypted, false); | ||
TestLogHandler::new().assert_logs_contain_in_order(vec![ | ||
"DbMigrator: Database successfully migrated from version 9 to 10", | ||
]); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I'm all okay with this test. But on the other hand, it also isn't exactly as I would expect it to be. I noticed you got inspired from your own migration "8 to 9", and the truth is there it was where you've begun writing in a different style than in which the other migrations are done.
I understand these are almost cosmetics but still, maybe you could have some time to do these simple changes.
So again, for it's better structured like that, we first ran all the migration but yours, where we stopped, and left the database in the state as it would be if you were truly in need of this last migration. Let's say from 0 to 9.
Then, the only thing left for the act is to run the function
initialize_to_version
where you specify the target as "10".This way the test is clear about what it is testing.
The process of gradual migrations is heavily tested on its own, separately. Here I could argue you're not testing the code added but rather the machinery around, which doesn't sound right.
Therefore I'd recommend you not to assert on logs for these database migrations unless it is an actual new log straight from within your implementation of that new migration. Which this is not the case. My opinion is the test would be better free of this massive log assertion.
I want to ask you to implement my ideas here (9 to 10) as well as for the previous migration (8 to 9). Thank you 🙏