MQTT test helpers to ease MqttMessage assertions

[albinsuresh]

Proposed changes

MQTT test helpers to ease MqttMessage assertions in converter unit tests that wants to assert a converted Vec<MqttMessage> against some expected messages.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[albinsuresh]

Instead of restricting the inputs to &mut Vec<MqttMessage>, I'd like to open up this API to accept any iterable that supports in-place removal of items from the source collection. It is taking an &mut input to remove all matched items from the input and leave the remaining items in the source. It is done this way to support the re-use of the source for further assertions, like the following sequence:

let messages = // Converted Vec<Message>
assert_messages_contains_str(
            &mut messages, ...)  // Initial messages
assert_messages_includes_json(
            &mut messages, ...)  // Remaining messages

So, all suggestions welcome.

[jarhodes314]

I think what you're looking for here is just to take an iterator directly and call .next() instead of .remove(0). That would also allow you to combine removing with checking whether the iterator is empty, e.g. the body of this function could become:

    for expected_msg in expected {
        if let Some(message) = messages.next() {
              assert_message_contains_str(&message, expected_msg);
        } else {
             panic!(
                  "Input messages exhausted while expecting message on topic: {:?} with payload: {:?}",
                  expected_msg.0.as_ref(), expected_msg.1
              )
        }
    }

Personally, I find a function called assert_... that mutates its input to be really weird. Additionally when I read assert_messages_contains_str, it sounds like it checks for any message in the list that matches a single expectation, rather than comparing the first N elements of the list with a list of N expectations. I wonder whether asserting against an array is all that useful. I don't know in detail what we are currently testing that could make use of this, but it feels like if we're asserting that more than a small handful of messages exist in a single test, that's probably indicative that there's a larger problem with the test being too complicated (which may itself indicate that the code being tested is hard to test).

I find the following considerably simpler and clearer as far as test code is concerned (and it means far less test helper code). Obviously if we don't have as many messages as expected, this won't give as clear an error message in the test, but it will still tell us which assertion failed.

let messages = messages.into_iter();

assert_message_contains_str(
    messages.next().unwrap(),
    (
        "c8y/s/us",
        "101,test-device:device:child1,child1,thin-edge.io-child",
    ),
);

assert_message_includes_json(
    messages.next().unwrap(),
    (
        "c8y/measurement/measurements/create",
        json!({
            "externalSource":{
                "externalId":"test-device:device:child1",
                "type":"c8y_Serial"
            },
            "temp":{
                "temp":{
                    "value":1.0
                }
            },
            "time":"2021-11-16T17:45:40.571760714+01:00",
            "type":"ThinEdgeMeasurement"
        }),
    ),
);

[didier-wenzek]

I agree with @jarhodes314 that the name assert_messages_contains_str is misleading.

I also wonder why tedge_actores::tedge_helpers::assert_received_matching cannot be used instead of a specific assert_messages_contains_str

[albinsuresh]

@jarhodes314

I think what you're looking for here is just to take an iterator directly and call .next() instead of .remove(0)

Yeah, this would have been possible if all the expected messages were provided together as the input and then I could have iterated over the entire collection. But, the limitation here is that, I couldn't provide all the inputs together in a single expected array as some of them were just plain strings and others were JSON values. This is why we're doing partial matches of the whole collection in different iterations with a combination of assert_messages_contains_str and assert_messages_contains_json. One way to avoid this is to have an enum as follows to represent the expected messages and accept an array of these for the assertions:

pub enum MqttPayload {
    StringMessage(String),
    JsonMessage(serde_json::Value),
    Skip(usize),
}

pub fn assert_messages<I, S>(messages: &mut Vec<MqttMessage>, expected: I)
where
    I: for<'a> IntoIterator<Item = (&'static str, MqttPayload)>,
{ ... }

Personally, I find a function called assert_... that mutates its input to be really weird

Yeah, I agree, especially for an input like a Vec. I was just trying to replicate the same API style that we had for the existing assert_received_xxx functions. That probably wasn't the best idea.

Additionally when I read assert_messages_contains_str, it sounds like it checks for any message in the list that matches a single expectation, rather than comparing the first N elements of the list with a list of N expectations

Agree with that too. I'll try to come up with a better name. Naming suggestions welcome.

I wonder whether asserting against an array is all that useful. I don't know in detail what we are currently testing that could make use of this, but it feels like if we're asserting that more than a small handful of messages exist in a single test, that's probably indicative that there's a larger problem with the test being too complicated (which may itself indicate that the code being tested is hard to test).

Yeah, we've got a few tests that assert upto 6 messages(all the init messages generated by the mapper). Even in most normal tests, we typically assert upto 4-5 messages(usually a device registration TEdge JSON message along with its SmartREST mapping followed by the actual converted message(s)).

I find the following considerably simpler and clearer as far as test code is concerned

Looking at that code, yes, I'd also agree with you. But, with the above proposed enum combined with the inbuilt Into impls that you suggested for those enum variants, I think I can provide a better API that can accept the entire list of expected inputs.

@didier-wenzek

I also wonder why tedge_actores::tedge_helpers::assert_received_matching cannot be used instead of a specific assert_messages_contains_str

If we're asserting messages in isolation, as shown in his sample, yes this definitely is an option. The goal of the helper function is to even eliminate the need for the user to provide the matching logic, just as a list out desired output messages.

[albinsuresh]

Not too bothered about the perf of these remove calls (because of the shifting of elements in the vec after each call), as this is a test helper. But, if there's a better way, lemme know.

[codecov]

Codecov Report

Merging #2342 (278ee03) into main (2724cc9) will increase coverage by 0.0%.
Report is 13 commits behind head on main.
The diff coverage is 88.4%.

❗ Current head 278ee03 differs from pull request most recent head 62095c5. Consider uploading reports for the commit 62095c5 to get more accurate results

Additional details and impacted files

Files	Coverage Δ
crates/extensions/c8y_mapper_ext/src/converter.rs	79.4% <97.6%> (+0.2%)	⬆️
...ates/extensions/tedge_mqtt_ext/src/test_helpers.rs	86.3% <82.8%> (-6.8%)	⬇️

... and 5 files with indirect coverage changes

[rina23q]

Looks good, left minor update comments

Edited: I withdrew some incorrect suggestions. If you see them from some history, please ignore them.

[rina23q]

This Debug can be removed I think.

[jarhodes314]

As discussed in #2342 (comment), I'm not convinced by the value of the functions that assert against arrays, and I think the looping could be greatly simplified in any case.

[jarhodes314]

It seems a bit arbitrary to me here that we accept anything that implements AsRef<str> here for the topic and the payload, but the two have to be the same concrete type.

Suggested change

	pub fn assert_message_contains_str<S>(message: &Message, expected: (S, S))
	where
	S: AsRef<str> + Debug,
	pub fn assert_message_contains_str(message: &Message, expected: (impl AsRef<str>, impl AsRef<str>))

would resolve this (as would adding a second type parameter. Given this is test code, I would argue we maybe don't need the complexity of impl AsRef<str> at all, most assertions are going to be static (so &str is what we're passing in anyway), and if we have String, it's trivial to convert to &str manually.

[albinsuresh]

Yeah, they didn't have to be the same type as implied by (S, S). I did that just to avoid the overkill of another generic parameter, as the inputs were always static str values. So, yes, &str probably is the best/simplest choice here.

[PradeepKiruvale]

LGTM

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
318	0	3	318	100	53m44.626s

[jarhodes314]

The introduction of MessagePayloadMatcher has greatly improved the value of the iterator-based assertion. It still feels a little bit clunky from the caller's perspective (quite a bit of nesting, and calling .into()).

For me, the ideal solution would probably be a builder-esque pattern, where the tuple-construction and conversion is completely hidden. Here's an example of what I mean:

#[test]
fn some_mqtt_test() {
    let messages = run_some_code();

    MessageExpectations::new()
        .expect("c8y/s/us", "A string message")
        .expect("some/json", json!({ "an": "object" }))
        .assert(messages);
}

#[derive(Default, Debug)]
pub struct MessageExpectations {
    inner: Vec<(String, MessagePayloadMatcher)>,
    consumed: bool,
}

impl MessageExpectations {
    #[must_use]
    pub fn new() -> Self {
        Default::default()
    }

    #[must_use]
    pub fn expect(mut self, topic: &str, payload: impl Into<MessagePayloadMatcher>) -> Self {
        self.inner.push((topic.to_owned(), payload.into()));
        self
    }

    pub fn assert(mut self, messages: impl IntoIterator<Item = MqttMessage>) {
        self.consumed = true;
        // body of assert_messages_matching here
    }
}

impl Drop for MessageExpectation {
    fn drop(&mut self) {
        if !self.consumed && !std::thread::panicking() {
            panic!("{self:?} dropped without `.assert` being called");
        }
    }
}

[jarhodes314]

Minor:

Suggested change

	assert_messages_matching(
	assert_messages_match(

[jarhodes314]

This combination of loop and match feels clunky. I think that a lot of the nesting and complexity can go away with something like:

Suggested change

	let mut messages_iter = messages.into_iter();
	let mut expected_iter = expected.into_iter();
	loop {
	match (messages_iter.next(), expected_iter.next()) {
	(Some(message), Some(expected_msg)) => {
	let message_topic = &message.topic.name;
	let expected_topic = expected_msg.0;
	match expected_msg.1 {
	MessagePayloadMatcher::StringMessage(str_payload) => {
	assert_message_contains_str(message, (expected_topic, str_payload))
	}
	MessagePayloadMatcher::JsonMessage(json_payload) => {
	assert_message_includes_json(message, (expected_topic, json_payload))
	}
	MessagePayloadMatcher::Empty => {
	assert_eq!(
	message_topic, expected_topic,
	"Received message on topic: {} instead of {}",
	message_topic, expected_topic
	);
	assert!(
	message.payload_bytes().is_empty(),
	"Received non-empty payload while expecting empty payload on {}",
	message_topic
	)
	}
	MessagePayloadMatcher::Skip => {
	assert_eq!(
	message_topic, expected_topic,
	"Received message on topic: {} instead of {}",
	message_topic, expected_topic
	);
	// Skipping payload validation
	}
	}
	}
	(None, Some(expected_msg)) => {
	panic!(
	"Input messages exhausted while expecting message on topic: {:?}",
	expected_msg.0
	)
	}
	(Some(message), None) => {
	panic!(
	"Additional message received than expected on topic: {:?}",
	message.topic.name
	)
	}
	_ => return,
	}
	}
	let mut messages = messages.into_iter();
	for expected_msg in expected {
	let Some(message) = messages.next() else {
	panic!(
	"Input messages exhausted while expecting message on topic: {:?}",
	expected_msg.0
	)
	};
	let message_topic = &message.topic.name;
	let expected_topic = expected_msg.0;
	match expected_msg.1 {
	MessagePayloadMatcher::StringMessage(str_payload) => {
	assert_message_contains_str(message, (expected_topic, str_payload))
	}
	MessagePayloadMatcher::JsonMessage(json_payload) => {
	assert_message_includes_json(message, (expected_topic, json_payload))
	}
	MessagePayloadMatcher::Empty => {
	assert_eq!(
	message_topic, expected_topic,
	"Received message on topic: {} instead of {}",
	message_topic, expected_topic
	);
	assert!(
	message.payload_bytes().is_empty(),
	"Received non-empty payload while expecting empty payload on {}",
	message_topic
	)
	}
	MessagePayloadMatcher::Skip => {
	assert_eq!(
	message_topic, expected_topic,
	"Received message on topic: {} instead of {}",
	message_topic, expected_topic
	);
	// Skipping payload validation
	}
	}
	}
	if let Some(message) = messages.next() {
	panic!(
	"Additional message received without expectation on topic: {:?}",
	message.topic.name
	);
	}

[albinsuresh]

Just a matter of style, I guess. For some reason, I still have a little preference for the existing style where all the match combinations like None, Some() and Some(), None are all laid out explicitly and even the panic! cases are part of those cases. But, if other also prefer this style with less nesting, I'm happy to change it.

[jarhodes314]

Personally, I find it really quite difficult to read (and modify) "diagonal" code (e.g. where the 8 lines starting from loop { cover 6 different indentation levels). In this case, what's going on is very simple, so it doesn't a huge difference. If you want the loop because it's explicit, it would make sense to replace the catch-all (i.e. _ => ... branch) with (None, None) => ... as that's what we care about here, and it reduces the mental overhead of having to remember exactly what we have and haven't handled (and catches potential future errors if someone decides to delete them). Also, the distinction here is pretty obvious, but in a long function, it can be quite difficult to distuingish between the various cases that arise in nested matches.

The main idea requested has been implemented

[albinsuresh]

For me, the ideal solution would probably be a builder-esque pattern, where the tuple-construction and conversion is completely hidden. Here's an example of what I mean:

#[test]
fn some_mqtt_test() {
    let messages = run_some_code();

    MessageExpectations::new()
        .expect("c8y/s/us", "A string message")
        .expect("some/json", json!({ "an": "object" }))
        .assert(messages);
}

On a first look, I was also quite impressed with that style, eliminating the need for those explicit into() calls. But, on a second look, I'm not quite sure if it saves a lot of typing for the developer. Looks like we eliminated the need to call .into() and replaced it with calls to expect. So, there isn't as much improvement as I initially thought. The equivalent of the same test with the proposed APIs would have been this:

#[test]
fn some_mqtt_test() {
    let messages = run_some_code();

    assert_messages_match(
            &messages,
            [
                ("c8y/s/us", "A string message".into()),
                ("some/json", json!({ "an": "object" }).into()),
            ],
    );
}

I find them both to be equally concise. I may be biased here because of the influnce of the older APIs. So, I'll let the immediate consumers @PradeepKiruvale and @rina23q judge which style they prefer and I'll proceed accordingly as the priority is to make it available ASAP to them.

[PradeepKiruvale]

For me, the ideal solution would probably be a builder-esque pattern, where the tuple-construction and conversion is completely hidden. Here's an example of what I mean:

#[test]
fn some_mqtt_test() {
    let messages = run_some_code();

    MessageExpectations::new()
        .expect("c8y/s/us", "A string message")
        .expect("some/json", json!({ "an": "object" }))
        .assert(messages);
}

On a first look, I was also quite impressed with that style, eliminating the need for those explicit into() calls. But, on a second look, I'm not quite sure if it saves a lot of typing for the developer. Looks like we eliminated the need to call .into() and replaced it with calls to expect. So, there isn't as much improvement as I initially thought. The equivalent of the same test with the proposed APIs would have been this:

#[test]
fn some_mqtt_test() {
    let messages = run_some_code();

    assert_messages_match(
            &messages,
            [
                ("c8y/s/us", "A string message".into()),
                ("some/json", json!({ "an": "object" }).into()),
            ],
    );
}

I find them both to be equally concise. I may be biased here because of the influnce of the older APIs. So, I'll let the immediate consumers @PradeepKiruvale and @rina23q judge which style they prefer and I'll proceed accordingly as the priority is to make it available ASAP to them.

For me, both look good. In the MessageExpectations::new() case the subsequent messages are built using the expect call, this is a bit confusing with the expect for panics. If we can change this with message then, it seems good because the same call be used to create the messages and assert them.

[jarhodes314]

I find them both to be equally concise. I may be biased here because of the influnce of the older APIs. So, I'll let the immediate consumers @PradeepKiruvale and @rina23q judge which style they prefer and I'll proceed accordingly as the priority is to make it available ASAP to them.

Yeah, the difference is less bad than I'd first thought, and I haven't had good thoughts as to the names of the methods. The builder API makes it simpler to modify how we assert a message (starts with, contains, exact match) in the future if useful, but you're right, it really doesn't offer us much value now. Not to mention the complexity we avoid with more subtle implementation details (like the drop implementation and must_use.

[rina23q]

For me, the ideal solution would probably be a builder-esque pattern, where the tuple-construction and conversion is completely hidden. Here's an example of what I mean:

#[test]
fn some_mqtt_test() {
    let messages = run_some_code();

    MessageExpectations::new()
        .expect("c8y/s/us", "A string message")
        .expect("some/json", json!({ "an": "object" }))
        .assert(messages);
}

On a first look, I was also quite impressed with that style, eliminating the need for those explicit into() calls. But, on a second look, I'm not quite sure if it saves a lot of typing for the developer. Looks like we eliminated the need to call .into() and replaced it with calls to expect. So, there isn't as much improvement as I initially thought. The equivalent of the same test with the proposed APIs would have been this:

#[test]
fn some_mqtt_test() {
    let messages = run_some_code();

    assert_messages_match(
            &messages,
            [
                ("c8y/s/us", "A string message".into()),
                ("some/json", json!({ "an": "object" }).into()),
            ],
    );
}

I find them both to be equally concise. I may be biased here because of the influnce of the older APIs. So, I'll let the immediate consumers @PradeepKiruvale and @rina23q judge which style they prefer and I'll proceed accordingly as the priority is to make it available ASAP to them.

Tbh, for me both look quite same. If I had to choose, I would prefer the MessageExpectations way because it doesn't require tuples. However, really both are fine. I'd say choose the one with simpler implementation behind the API.

[albinsuresh]

I would prefer the MessageExpectations way because it doesn't require tuples

Remove the expect keyword from expect("c8y/s/us", "A string message") function call and the remaining part is like a tuple 😉

On a serious note, since everyone agrees that the differences are minor, I'll just merge the current API to avoid further delays. It also makes the tests look consistent with the ones that we already have in c8y_mapper_ext::tests.rs, enabling easier portability.