-
Notifications
You must be signed in to change notification settings - Fork 166
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
Pass args to coverage and profiler #2592
base: master
Are you sure you want to change the base?
Conversation
commit-id:544babad
commit-id:40cdedbd
commit-id:ea228d4e
ForgeSubcommand::Test { mut args } => { | ||
// clap can capture the first flag in test filter, | ||
// let's move it to coverage_or_profiler_args | ||
if let Some(test_filter) = &args.test_filter { | ||
if test_filter.starts_with('-') { | ||
args.coverage_or_profiler_args.insert(0, test_filter.into()); | ||
args.test_filter = None; | ||
} | ||
} |
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 suppose that such heuristic can generate problems in the future, and introducing such condition disturbs the readability of the code. Maybe we should just add coverage subcommand to catch arguments in simpler and cleaner way? What do you think?
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 we could at least check if --coverage
or --build-profile
were actually passed. Because I think this heuristic should be only enabled in this case, otherwise, we shouldn't be doing anything with the filter.
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.
We could consider some warning when it runs, but not sure what it would be,
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 we could at least check if --coverage or --build-profile were actually passed. Because I think this heuristic should be only enabled in this case, otherwise, we shouldn't be doing anything with the filter.
yes, it's the correct one, but it is going to be the same thing as if there was just a subcommand.
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 suppose that such heuristic can generate problems in the future, and introducing such condition disturbs the readability of the code. Maybe we should just add coverage subcommand to catch arguments in simpler and cleaner way? What do you think?
I guess we were discussing this earlier i don't know why we bring it here now and block it with the request changes 😅
We could consider some warning when it runs, but not sure what it would be
There shouldn't be any warning. snforge test --coverage --my-coverage-flag
is perfectly fine and user doesn't do anything wrong and yet it caputers --my-coverage-flag in test filter
I think we could at least check if --coverage or --build-profile were actually passed. Because I think this heuristic should be only enabled in this case, otherwise, we shouldn't be doing anything with the filter.
What about some meaningful validation of argument passed to filter? I don't think we should allow passing test filter argument starting -
.
ForgeSubcommand::Test { mut args } => { | ||
// clap can capture the first flag in test filter, | ||
// let's move it to coverage_or_profiler_args | ||
if let Some(test_filter) = &args.test_filter { | ||
if test_filter.starts_with('-') { | ||
args.coverage_or_profiler_args.insert(0, test_filter.into()); | ||
args.test_filter = None; | ||
} | ||
} |
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 we could at least check if --coverage
or --build-profile
were actually passed. Because I think this heuristic should be only enabled in this case, otherwise, we shouldn't be doing anything with the filter.
ForgeSubcommand::Test { mut args } => { | ||
// clap can capture the first flag in test filter, | ||
// let's move it to coverage_or_profiler_args | ||
if let Some(test_filter) = &args.test_filter { | ||
if test_filter.starts_with('-') { | ||
args.coverage_or_profiler_args.insert(0, test_filter.into()); | ||
args.test_filter = None; | ||
} | ||
} |
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.
We could consider some warning when it runs, but not sure what it would be,
Closes software-mansion/cairo-coverage#80
Introduced changes
cairo-coverage
andcairo-profiler
--coverage
and--build-profile
togetherChecklist
CHANGELOG.md