Skip to content
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

Fix mean on empty arrays #2612

Merged
merged 3 commits into from Aug 4, 2022
Merged

Conversation

Rubilmax
Copy link
Contributor

@Rubilmax Rubilmax commented Aug 4, 2022

Motivation

The gas report of my test suite is panicking when providing FOUNDRY_FUZZ_RUNS=0, because it's computing the mean of an empty array (&func.calls)

Solution

Check if the array is empty in calc::mean

@onbjerg onbjerg added the T-bug Type: bug label Aug 4, 2022
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 😄 let's add a small test to ensure we don't break it?

also welcome to github, thanks for submitting your first pr ever to foundry 🥳

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks,

smol style nit

Comment on lines 12 to 17
let len = values.len();
if len > 0 {
values.iter().copied().fold(U256::zero(), |sum, val| sum + val.into()) / len
} else {
U256::zero()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer

if values.is_empty() {
  return U256::zero()
}

here

@Rubilmax
Copy link
Contributor Author

Rubilmax commented Aug 4, 2022

lgtm 😄 let's add a small test to ensure we don't break it?

also welcome to github, thanks for submitting your first pr ever to foundry 🥳

Ahah, I'm learning Rust
Currently it seems there are no tests for calc (or I can't find them?)
It may take me a while before adding such a test, but I may succeed

@Rubilmax Rubilmax requested a review from mattsse August 4, 2022 15:04
@onbjerg
Copy link
Member

onbjerg commented Aug 4, 2022

@Rubilmax the tests are in the test module in the file itself at the bottom:

#[cfg(test)]
mod tests {
use super::*;
#[test]
fn calc_mean() {
let values = [0u64, 1u64, 2u64, 3u64, 4u64, 5u64, 6u64];
let m = mean(&values);
assert_eq!(m, 3u64.into());
}
#[test]
fn calc_median() {
let mut values = vec![29, 30, 31, 40, 59, 61, 71];
values.sort();
let m = median_sorted(&values);
assert_eq!(m, 40);
}
#[test]
fn calc_median_even() {
let mut values = vec![80, 90, 30, 40, 50, 60, 10, 20];
values.sort();
let m = median_sorted(&values);
assert_eq!(m, 45);
}
}

The #[cfg(test)] attribute means that the module is only compiled during tests - the #[test] attribute on each function in there marks the function as a test 😄 Let me know if you need some help building the test

More in the Rust book: https://doc.rust-lang.org/book/ch11-01-writing-tests.html

Edit: You can basically just copy one of the tests and pass in an empty array

@Rubilmax
Copy link
Contributor Author

Rubilmax commented Aug 4, 2022

@Rubilmax the tests are in the test module in the file itself at the bottom:

#[cfg(test)]
mod tests {
use super::*;
#[test]
fn calc_mean() {
let values = [0u64, 1u64, 2u64, 3u64, 4u64, 5u64, 6u64];
let m = mean(&values);
assert_eq!(m, 3u64.into());
}
#[test]
fn calc_median() {
let mut values = vec![29, 30, 31, 40, 59, 61, 71];
values.sort();
let m = median_sorted(&values);
assert_eq!(m, 40);
}
#[test]
fn calc_median_even() {
let mut values = vec![80, 90, 30, 40, 50, 60, 10, 20];
values.sort();
let m = median_sorted(&values);
assert_eq!(m, 45);
}
}

The #[cfg(test)] attribute means that the module is only compiled during tests - the #[test] attribute on each function in there marks the function as a test 😄 Let me know if you need some help building the test

More in the Rust book: https://doc.rust-lang.org/book/ch11-01-writing-tests.html

Edit: You can basically just copy one of the tests and pass in an empty array

Thank you, done!

@gakonst gakonst merged commit 6e34042 into foundry-rs:master Aug 4, 2022
@Rubilmax Rubilmax deleted the Rubilmax/fix-mean-calc branch August 4, 2022 15:39
iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
* Fix mean on empty arrays

* Update code style

* Add empty slice tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mean is panicking on empty arrays
4 participants