Conversation
Introduce proptest and convert example tests to the property-testing framework. General behaviour is enforced and the tests are able to catch non-hardcoded cases. Signed-off-by: Leonardo Forchini <leonardo.forchini@nutanix.com>
885e7a6 to
24d431f
Compare
tmakatos
left a comment
tmakatos
left a comment
There was a problem hiding this comment.
I've only reviewed half of it. Can we standardise test naming? I find test_foo_XXX very useful especially when grepping to see what tests exists for a function. That requires fixing names of existing tests.
| /// Test that vQ round-robin mapping spreads queues across IOThreads | ||
| /// as evenly as possible. | ||
| proptest! { | ||
| #[test] |
| prop_assert!(max - min <= 1); | ||
| } | ||
|
|
||
| #[test] |
| /// as evenly as possible. | ||
| proptest! { | ||
| #[test] | ||
| fn round_robin_partitions_queues( |
There was a problem hiding this comment.
Could you add some inline comments explaining what's been artificially constructed and what's being tested?
| let topo = QemuTopology::new(body); | ||
| assert_eq!(topo.iothreads, vec!["iot0"]); | ||
| assert_eq!(topo.iothread_tids.get("iot0"), Some(&321)); | ||
| assert_eq!(topo.device_path, "/machine/peripheral/scsi0"); |
|
|
||
| /// Test that a repeated IOThread id keeps the tid from the last line. | ||
| #[test] | ||
| fn topology_keeps_the_last_tid_for_a_repeated_id() { |
There was a problem hiding this comment.
Is that even possible? Shouldn't the backend fail in this case?
| } | ||
| other => panic!("unexpected err: {other:?}"), | ||
| } | ||
| fn blockstats_reads_one_device() { |
There was a problem hiding this comment.
nit, shouldn't this be named test_parse_blockstats_XXX? Same or above.
| /// other I/O. | ||
| #[test] | ||
| fn parse_blockstats_sums_across_devices() { | ||
| fn blockstats_sums_devices() { |
| EngineTickContext::default() | ||
| } | ||
|
|
||
| async fn set_observation( |
There was a problem hiding this comment.
nit, add function description since you're touching this file anyway?
|
|
||
| #[fixture] | ||
| fn engine() -> ThresholdEngine { | ||
| fn validation_engine(polls: u32) -> ThresholdEngine { |
There was a problem hiding this comment.
Why get rid of fixtures? They make the body of the test slightly more focused on the behaviour being tested rather than the setting things up.
| engine.evaluate(&instance, &context()).await, | ||
| ScaleAction::Revert(3) | ||
| ); | ||
| fn nearly_eq(left: f64, right: f64) -> bool { |
There was a problem hiding this comment.
Document how much is acceptable?
2 participants
Introduce proptest and convert example tests to the property-testing
framework. General behaviour is enforced and the tests are able to catch
non-hardcoded cases.
Signed-off-by: Leonardo Forchini leonardo.forchini@nutanix.com
Stack created with GitHub Stacks CLI • Give Feedback 💬