Add CLI flags -n, -t, T and -p

[AntoniosBarotsis]

This closes #13.

Only added @jhodapp as a reviewer because that's who I was in contact with when working on this, feel free to add anyone else!

The relevant slack thread can be found here.

Added

  -n, --post-amount <POST_AMOUNT>
          The number of sensor readings to post. [DEFAULT: 1]
  -t, --time-per-post <TIME_PER_POST_S>
          The time between each sensor reading post (in seconds). [DEFAULT: 10]
  -T, --total-time <TOTAL_TIME_S>
          The total time over which all the sensor reading posts must be sent (in seconds, alternative to -t)
  -p, --num-threads <NUM_THREADS>
          The number of threads to spawn. The workload will be cloned to each thread, not divided. [DEFAULT: 1]

Testing

From some manual testing, everything seems fine from what I can tell. It would be great if you could play around with it for a little bit to verify both that it works and that it feels as you'd expect (things like flag usage, default behavior, etc).

You can run it with cargo r -- -h or you can install it locally with cargo install --path . and then ambi_mock_client -h.

Other Changes

This PR also upgraded to the latest version of clap and a bunch of other dependencies, moved a bunch of stuff around and added a logger.

[AntoniosBarotsis]

I think in this case it's a bit more trivial as all of the stuff I upgraded are minor versions of popular crates which you can confidently assume have no breaking changes or anything but more importantly because this project is a standalone binary. If it was a library (especially one in an embedded context like the wifi drivers you are developing for instance) I would've definitely not made these so lightly but in this case, I do believe they are somewhat trivial.

That said, I should've definitely mentioned my reasoning as to why I made them in the desc PR's description, thanks for pointing that out!

[jhodapp]

Yeah you're welcome, agreed that it's pretty trivial in this case which is why I am not asking that you remove them from this PR and out into a separate one. Thanks for understanding.

More review on the content of your PR soon...

[calebbourg]

@AntoniosBarotsis This looks great! Thank you for adding this. Just left a few questions/ideas.

[AntoniosBarotsis]

@calebbourg Just pushed the changes, let me know if everything is resolved properly :)

[calebbourg]

Love it!

[jhodapp]

Looking great, made a few recommendations inline. Open to discussing any of them.

[jhodapp]

@AntoniosBarotsis I think we should also change nearly ever mention of a request to be post or something contextually relevant. Does it even make sense to have a requests.rs now? Consider changing it to sensor_posts.rs as on idea.

[AntoniosBarotsis]

I've left a few mentions of request where I feel like it makes sense, such as here. Let me know what you think.

[AntoniosBarotsis]

@jhodapp, probably missed my comment here :)

[jhodapp]

Definitely a step in the right direction, made a suggestion to further rename things.

[AntoniosBarotsis]

I added this section here which will make sure to log the response message as an error if the response code is not in the 200-299 range, makes it easier to immediately identify if something did not work.

[jhodapp]

Looks like the tests don't currently pass, I'm seeing 2 failures. Are you seeing these as well?

running 4 tests
test sensor_posts::tests::test_invalid_num_threads_low ... FAILED
test sensor_posts::tests::test_invalid_num_threads_high ... FAILED
test data::tests::air_purity_from_value_returns_correct_enum ... ok
test tests::verify_cli ... ok

failures:

---- sensor_posts::tests::test_invalid_num_threads_low stdout ----
thread 'sensor_posts::tests::test_invalid_num_threads_low' panicked at 'assertion failed: req_scheduler.is_err()', src/sensor_posts.rs:224:9

---- sensor_posts::tests::test_invalid_num_threads_high stdout ----
thread 'sensor_posts::tests::test_invalid_num_threads_high' panicked at 'assertion failed: req_scheduler.is_err()', src/sensor_posts.rs:233:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    sensor_posts::tests::test_invalid_num_threads_high
    sensor_posts::tests::test_invalid_num_threads_low

test result: FAILED. 2 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

[AntoniosBarotsis]

Ah, should've run those again (probably a good enough excuse to set up a basic CI for this).

Caleb suggested I move some of the validation (for what is considered a valid number of threads) out of the code and use Clap's validators instead. The tests don't fail anymore because the check would normally happen higher up the stack (as in, if you indeed try to run it with 0 or 11 threads which is what the tests are doing, it will fail with an error as it should).

I'll fix the tests later today and make sure they run in GitHub actions from now on.

[jhodapp]

Sounds great, I look forward to seeing your updates!

[AntoniosBarotsis]

Ok so, fixed the tests, they look like this now:

let cli = Cli::try_parse_from(vec!["", "-p", "0"]);
assert!(cli.is_err());

let cli_err = cli.unwrap_err();
assert_eq!(cli_err.kind(), ErrorKind::ValueValidation);

I just ensure that clap is failing to parse the input as we consider this amount of threads invalid.

I also added the basic workflow I mentioned earlier although I believe it won't run until it's merged which is a bit annoying.

Sorry for the many back and forths, last request review is this (waiting for reply) and everything should be fine after that!

[jhodapp]

Looks great Tony, thanks again for this! Tests pass for me now.