-
Notifications
You must be signed in to change notification settings - Fork 335
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
Support all runtimes in perf-tests #940
Conversation
node/service/src/lib.rs
Outdated
pub enum RuntimeVariant { | ||
Moonbeam, | ||
Moonriver, | ||
Moonbase, | ||
Unrecognized, | ||
} | ||
|
||
impl RuntimeVariant { | ||
pub fn from_chain_spec(chain_spec: &Box<dyn ChainSpec>) -> Self { | ||
match chain_spec { | ||
spec if spec.is_moonbeam() => Self::Moonbeam, | ||
spec if spec.is_moonriver() => Self::Moonriver, | ||
spec if spec.is_moonbase() => Self::Moonbase, | ||
_ => Self::Unrecognized, | ||
} | ||
} | ||
} |
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.
This mostly exists because just about everything else related to the runtime variants and chain specs are not Copy
, Clone
, etc. I'm open to other suggestions (esp. @librelois)
node/service/src/lib.rs
Outdated
pub fn for_chain_spec(chain_spec: &Box<dyn ChainSpec>) -> Self { | ||
match chain_spec { | ||
spec if spec.is_moonbeam() => Self::moonbeam(), | ||
spec if spec.is_moonriver() => Self::moonriver(), | ||
spec if spec.is_moonbase() => Self::moonbase(), | ||
_ => panic!("invalid chain spec"), | ||
} | ||
} |
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'm not using this in the PR, but I found it very useful when I first wrote it (it helps avoid big #[cfg...]
blocks of code). OTOH, I'm not a fan of dead code...
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.
So, please remove this unused code
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.
Fixed in 7a35f70
node/perf-test/src/lib.rs
Outdated
long = "tests", | ||
help = "Comma-separated list of tests to run (if omitted, runs all tests)" | ||
)] | ||
pub tests: Option<String>, |
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.
pub tests: Option<String>, | |
pub tests: Vec<String>, |
This will allow you to do --tests test1 --tests test2 --tests test3
which is not so bad if you give it a short option like -t
. I don't know how to get the syntax you described though.
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'm not sure why this showed up in this PR, it was introduced in a different one.
Anyway, the Vector<...>
trick sounds useful, but I added code to parse a comma-separated list (in that other PR) as the TODO suggested -- so I removed the TODO comment.
node/service/src/lib.rs
Outdated
pub fn for_chain_spec(chain_spec: &Box<dyn ChainSpec>) -> Self { | ||
match chain_spec { | ||
spec if spec.is_moonbeam() => Self::moonbeam(), | ||
spec if spec.is_moonriver() => Self::moonriver(), | ||
spec if spec.is_moonbase() => Self::moonbase(), | ||
_ => panic!("invalid chain spec"), | ||
} | ||
} |
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.
So, please remove this unused code
What does it do?
Supports all runtimes in
perf-tests
.TODO:
load_spec()
hackchain_spec
in CLI params