fix: unify environments between the two modes #59
Conversation
art049
force-pushed
the
env-fixes
branch
2 times, most recently
from
7d05444 to
0642648
Compare
adriencaccia
left a comment
adriencaccia
left a comment
There was a problem hiding this comment.
LGTM, only one small remark on where to define the RunnerMode linked to an Executor
| // Configure the environment | ||
| cmd.env( | ||
| cmd.envs(get_base_injected_env( | ||
| RunnerMode::Instrumentation, |
There was a problem hiding this comment.
Should we have the executor define its RunnerMode?
So that we do not have to hardcode it here, and instead have it as a method of the Executor trait
There was a problem hiding this comment.
this is not useful with the valgrind executor since it needs refactoring to be properly implemented. We could drill the mode into measure but that would be pointless.
| use crate::prelude::*; | ||
| use crate::run::instruments::mongo_tracer::MongoTracer; | ||
| use crate::run::{check_system::SystemInfo, config::Config}; |
There was a problem hiding this comment.
nit: import grouping
There was a problem hiding this comment.
can cargo fmt handle this?
There was a problem hiding this comment.
we could set this in our repos: https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=granula#Crate%5C%3A
There was a problem hiding this comment.
nevermind it's unstable and therefore not able to be put in rustfmt.toml, it needs to be set at IDE level 😢
@adriencaccia there was confusion between Mode and Executor, so I refactored this logic. I hadn't pulled it, so I redid the changes you made in the last commit, but in another way.