-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: create dump_config for mempool node config #164
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @ArniStarkware and the rest of your teammates on |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
===========================================
+ Coverage 49.40% 65.69% +16.29%
===========================================
Files 24 25 +1
Lines 927 930 +3
Branches 927 930 +3
===========================================
+ Hits 458 611 +153
+ Misses 446 293 -153
- Partials 23 26 +3 ☔ View full report in Codecov by Sentry. |
67eba04
to
166d248
Compare
ea5c83b
to
d619c04
Compare
166d248
to
43fc7d4
Compare
d619c04
to
3399d1a
Compare
43fc7d4
to
594b159
Compare
3399d1a
to
29637fa
Compare
594b159
to
0a3b6c1
Compare
29637fa
to
0385976
Compare
0a3b6c1
to
5925b99
Compare
0385976
to
a3b3603
Compare
5925b99
to
13b61d5
Compare
a3b3603
to
fed703c
Compare
13b61d5
to
151ea3c
Compare
be349d7
to
028449e
Compare
028449e
to
6907f31
Compare
ffca060
to
405a4cd
Compare
6907f31
to
fabbd25
Compare
405a4cd
to
11f6fed
Compare
fabbd25
to
745774e
Compare
11f6fed
to
828c4b2
Compare
745774e
to
3e40895
Compare
828c4b2
to
47dea97
Compare
3e40895
to
8419944
Compare
47dea97
to
7f9ecce
Compare
82e45d0
to
a3f86a1
Compare
a3f86a1
to
408fd02
Compare
0e64282
to
0626eb6
Compare
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.
Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)
crates/mempool_node/src/config/config_test.rs
line 55 at r2 (raw file):
#[test] fn test_dump_default_config() {
Only a question: Is this test checking that dump () is working OK?
crates/mempool_node/src/config/config_test.rs
line 62 at r2 (raw file):
insta::assert_json_snapshot!(dumped_default_config); assert!(default_config.validate().is_ok());
This line should come before "let dumped_default_config..." because if validation fails no need to check json snapshot.
Code quote:
assert!(default_config.validate().is_ok());
crates/mempool_node/src/config/config_test.rs
line 78 at r2 (raw file):
// Read the dumped config from the file. let from_code: serde_json::Value = serde_json::from_reader(File::open(tmp_file_path).unwrap()).unwrap();
I think that the next code should be enough:
look at " CODE SNIPPET (ii)"
Code quote (i):
// Create a temporary file and dump the default config to it.
let mut tmp_file_path = env::temp_dir();
tmp_file_path.push("cfg.json");
MempoolNodeConfig::default().dump_to_file(&vec![], tmp_file_path.to_str().unwrap()).unwrap();
// Read the dumped config from the file.
let from_code: serde_json::Value =
serde_json::from_reader(File::open(tmp_file_path).unwrap()).unwrap();
Code snippet (ii):
let from_code: serde_json::Value =
serde_json::to_value(MempoolNodeConfig::default().dump()).unwrap();
0626eb6
to
0bf3b02
Compare
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.
Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry, @lev-starkware, and @Yael-Starkware)
crates/mempool_node/src/config/config_test.rs
line 55 at r2 (raw file):
Previously, lev-starkware wrote…
Only a question: Is this test checking that dump () is working OK?
Yes.
- That the dump is working.
- That the dumped value is consistent with the latest dumped value. If these values are inconsistent, the developer can review the changes and fix the snapshot by following the instructions presented by
insta
.
crates/mempool_node/src/config/config_test.rs
line 62 at r2 (raw file):
Previously, lev-starkware wrote…
This line should come before "let dumped_default_config..." because if validation fails no need to check json snapshot.
Done.
crates/mempool_node/src/config/config_test.rs
line 78 at r2 (raw file):
Previously, lev-starkware wrote…
I think that the next code should be enough:
look at " CODE SNIPPET (ii)"
Done.
0bf3b02
to
09b146e
Compare
09b146e
to
26e115e
Compare
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.
Reviewed 3 of 8 files at r1, 5 of 5 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry and @Yael-Starkware)
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.
Reviewed 3 of 8 files at r1, 5 of 5 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)