-
Notifications
You must be signed in to change notification settings - Fork 115
feat(tests): allow RUST_LOG with multiple tags for native executables #2452
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
base: dev
Are you sure you want to change the base?
Changes from 1 commit
9cc04cf
fa67138
864e7e5
fda8099
9675bd8
31d9d69
a2dca34
66a293d
5c6569a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5487,11 +5487,12 @@ fn test_peer_time_sync_validation() { | |
| let timeoffset_tolerable = TryInto::<i64>::try_into(MAX_TIME_GAP_FOR_CONNECTED_PEER).unwrap() - 1; | ||
| let timeoffset_too_big = TryInto::<i64>::try_into(MAX_TIME_GAP_FOR_CONNECTED_PEER).unwrap() + 1; | ||
|
|
||
| let start_peers_with_time_offset = |offset: i64| -> (Json, Json) { | ||
| let start_peers_with_time_offset = |offset: i64, skip_seednodes_check: bool| -> (Json, Json) { | ||
| let (_ctx, _, bob_priv_key) = generate_utxo_coin_with_random_privkey("MYCOIN", 10.into()); | ||
| let (_ctx, _, alice_priv_key) = generate_utxo_coin_with_random_privkey("MYCOIN1", 10.into()); | ||
| let coins = json!([mycoin_conf(1000), mycoin1_conf(1000)]); | ||
| let bob_conf = Mm2TestConf::seednode(&hex::encode(bob_priv_key), &coins); | ||
| let mut bob_conf = Mm2TestConf::seednode(&hex::encode(bob_priv_key), &coins); | ||
| bob_conf.conf["skip_seednodes_check"] = skip_seednodes_check.into(); | ||
| let mut mm_bob = block_on(MarketMakerIt::start_with_envs( | ||
| bob_conf.conf, | ||
| bob_conf.rpc_password, | ||
|
|
@@ -5501,8 +5502,9 @@ fn test_peer_time_sync_validation() { | |
| .unwrap(); | ||
| let (_bob_dump_log, _bob_dump_dashboard) = mm_dump(&mm_bob.log_path); | ||
| block_on(mm_bob.wait_for_log(22., |log| log.contains(">>>>>>>>> DEX stats "))).unwrap(); | ||
| let alice_conf = | ||
| let mut alice_conf = | ||
| Mm2TestConf::light_node(&hex::encode(alice_priv_key), &coins, &[mm_bob.ip.to_string().as_str()]); | ||
| alice_conf.conf["skip_seednodes_check"] = skip_seednodes_check.into(); | ||
| let mut mm_alice = block_on(MarketMakerIt::start_with_envs( | ||
| alice_conf.conf, | ||
| alice_conf.rpc_password, | ||
|
|
@@ -5539,7 +5541,7 @@ fn test_peer_time_sync_validation() { | |
| }; | ||
|
|
||
| // check with small time offset: | ||
| let (bob_peers, alice_peers) = start_peers_with_time_offset(timeoffset_tolerable); | ||
| let (bob_peers, alice_peers) = start_peers_with_time_offset(timeoffset_tolerable, false); | ||
| assert!( | ||
| bob_peers["result"].as_object().unwrap().len() == 1, | ||
| "bob must have one peer" | ||
|
|
@@ -5550,7 +5552,7 @@ fn test_peer_time_sync_validation() { | |
| ); | ||
|
|
||
| // check with too big time offset: | ||
| let (bob_peers, alice_peers) = start_peers_with_time_offset(timeoffset_too_big); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how did this used to work before even tho the default behavior is setting
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It had worked before this PR because the node waited for log message 'Completed IAmrelay handling for peer' and this message always was added to the log when a peer connected (even if the peer was later rejected due to big time diff) |
||
| let (bob_peers, alice_peers) = start_peers_with_time_offset(timeoffset_too_big, true); // skip_seednodes_check = true as there should not be connected peers | ||
| assert!( | ||
| bob_peers["result"].as_object().unwrap().is_empty(), | ||
| "bob must have no peers" | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||||||||||||||
| use common::executor::Timer; | ||||||||||||||||||
| use common::log::LogLevel; | ||||||||||||||||||
| #[cfg(target_arch = "wasm32")] use common::log::LogLevel; | ||||||||||||||||||
| use common::{block_on, log, now_ms, wait_until_ms}; | ||||||||||||||||||
| use crypto::privkey::key_pair_from_seed; | ||||||||||||||||||
| use mm2_main::{lp_main, lp_run, LpMainParams}; | ||||||||||||||||||
|
|
@@ -12,7 +12,7 @@ use mm2_test_helpers::structs::{CreateNewAccountStatus, HDAccountAddressId, HDAc | |||||||||||||||||
| use serde_json::{self as json, Value as Json}; | ||||||||||||||||||
| use std::collections::HashMap; | ||||||||||||||||||
| use std::env::var; | ||||||||||||||||||
| use std::str::FromStr; | ||||||||||||||||||
| #[cfg(target_arch = "wasm32")] use std::str::FromStr; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// This is not a separate test but a helper used by `MarketMakerIt` to run the MarketMaker from the test binary. | ||||||||||||||||||
| #[test] | ||||||||||||||||||
|
|
@@ -21,15 +21,19 @@ fn test_mm_start() { test_mm_start_impl(); } | |||||||||||||||||
|
|
||||||||||||||||||
| pub fn test_mm_start_impl() { | ||||||||||||||||||
| if let Ok(conf) = var("_MM2_TEST_CONF") { | ||||||||||||||||||
|
Comment on lines
16
to
21
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we can merge these 2 funcs. as of now, doesn't seem that this indirection is useful. |
||||||||||||||||||
| if let Ok(log_var) = var("RUST_LOG") { | ||||||||||||||||||
| if let Ok(filter) = LogLevel::from_str(&log_var) { | ||||||||||||||||||
| log!("test_mm_start] Starting the MarketMaker..."); | ||||||||||||||||||
| let conf: Json = json::from_str(&conf).unwrap(); | ||||||||||||||||||
| let params = LpMainParams::with_conf(conf).log_filter(Some(filter)); | ||||||||||||||||||
| let ctx = block_on(lp_main(params, &|_ctx| (), "TEST".into(), "TEST".into())).unwrap(); | ||||||||||||||||||
| block_on(lp_run(ctx)) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| log!("test_mm_start] Starting the MarketMaker..."); | ||||||||||||||||||
| let conf: Json = json::from_str(&conf).unwrap(); | ||||||||||||||||||
| #[cfg(not(target_arch = "wasm32"))] | ||||||||||||||||||
| let filter = None; | ||||||||||||||||||
| #[cfg(target_arch = "wasm32")] | ||||||||||||||||||
| let filter = if let Ok(log_var) = var("RUST_LOG") { | ||||||||||||||||||
| LogLevel::from_str(&log_var).ok(); // Actually filter is used for wasm only. For native RUST_LOG is parsed by env_logger directly, allowing setting multiple targets | ||||||||||||||||||
mariocynicys marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||
| } else { | ||||||||||||||||||
| None | ||||||||||||||||||
| }; | ||||||||||||||||||
|
||||||||||||||||||
| let filter = if let Ok(log_var) = var("RUST_LOG") { | |
| LogLevel::from_str(&log_var).ok(); // Actually filter is used for wasm only. For native RUST_LOG is parsed by env_logger directly, allowing setting multiple targets | |
| } else { | |
| None | |
| }; | |
| // Actually filter is used for wasm only. For native RUST_LOG is parsed by env_logger directly, allowing setting multiple targets | |
| let filter = var("RUST_LOG").map(|l| LogLevel::from_str(&log_var).ok()); | |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1374,14 +1374,15 @@ impl MarketMakerIt { | |||||
| let executable = try_s!(env::args().next().ok_or("No program name")); | ||||||
| let executable = try_s!(Path::new(&executable).canonicalize()); | ||||||
| let log = try_s!(fs::File::create(&log_path)); | ||||||
| let envs_fixed = fix_rust_log_if_present(envs); | ||||||
| let child = try_s!(Command::new(executable) | ||||||
| .arg("test_mm_start") | ||||||
| .arg("--nocapture") | ||||||
| .current_dir(&folder) | ||||||
| .env("_MM2_TEST_CONF", try_s!(json::to_string(&conf))) | ||||||
| .env("MM2_UNBUFFERED_OUTPUT", "1") | ||||||
| .env("RUST_LOG", "debug") | ||||||
| .envs(envs.to_vec()) | ||||||
| .env("RUST_LOG", read_rust_log().as_str()) | ||||||
| .envs(envs_fixed.iter().map(|(k, v)| (k, v))) | ||||||
|
||||||
| .envs(envs_fixed.iter().map(|(k, v)| (k, v))) | |
| .envs(envs_fixed.iter())) |
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.
Thanks
Outdated
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.
if the rust_log was set, we would have two rust_logs set for the child process here. one from .env("RUST_LOG",...) and another from .envs(envs_fixed...). is this OK? can't we rely on our own only one of them, say the former.
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.
I think it's okay as envs fn according to the doc 'Inserts or updates multiple explicit environment variable mappings.'
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.
ummmm, i mean, there is no reason to provide it two times anyways.
if we can make it so fix_rust_log_if_present be remove_rust_log_if_present instead, we won't have repeated RUST_LOGs.
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.
I believe, the second RUST_LOG var (if present) would overwrite the first one in the inner 'set' of env vars
Outdated
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.
i guess that's a different check than what the funciton name tells, ha?
this queries connected peers, but in this function we wanna check connected seeds.
not sure if the old implementation used to check for seeds and not any peer (the log we used to wait for doesn't really point that it was waiting specifically for a seed).
it doesn't matter i think since in all test scenarios we have a seed peer connected to non-seed peer. we might just wanna rename the function to reflect the job it's doing.
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.
renamed,
tyvm for the notes
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.
didn't understand why we had this, but looks like it's some logic found in wait_for_log that was used before.
i think the fact that the rpc replied to us is enough to determine that mm2 is alive, so such a check isn't really needed.
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.
I think I'd better catch the error from self.rpc() so we can progress and stop with the known "MM process terminated prematurely" error
Uh oh!
There was an error while loading. Please reload this page.