tests: Remove hardcoded skips of certain number of messages from c8y mapper Rust unit tests

[Bravo555]

Proposed changes

Having hardcoded "skip N messages" in tests makes it harder to change what messages are published, e.g. in #4064 where adding 114 message after registration makes a bunch of tests fail because now instead of skipping N messages they need to skip N+1 messages.

This skipping was necessary as long as the tests were operating on regular MessageReceivers, which only provide an interface to receive a new message and have no way to store already received messages. I'd argue that while it's a useful foundation for expressing different types of receivers which are used in production code, it's probably a bad abstraction for tests where we can easily store incoming messages to a Vec as they come and then run asserts not on one message that we just received, but on all messages received.

Thus, the PR provides a new MockMqttBox (alternative proposed names: TestMqttBox/BufferedMqttBox) which wraps any receiver and stores its received messages. The assertions then run on all captures messages.

The assertions then can be made more flexible, but currently they still need to be adjusted to some of the intuitive usage patterns. For example, after asserting message m1 and then asserting message m2, the writer of the test probably expects that they asserted that m2 happens after m1. So for simple and common asserts we could update a cursor pointing to the least recently asserted message and only assert additional messages after that.

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. You can activate automatic signing by running just prepare-dev once)
• I ran just format as mentioned in CODING_GUIDELINES
• I used just check 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

[codecov]

Codecov Report

❌ Patch coverage is 88.09524% with 20 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...mqtt_ext/src/test_helpers/test_mqtt_box/asserts.rs	85.71%	7 Missing ⚠️
...dge_mqtt_ext/src/test_helpers/test_mqtt_box/mod.rs	91.66%	3 Missing and 4 partials ⚠️
...ates/extensions/tedge_mqtt_ext/src/test_helpers.rs	81.81%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
934	0	7	934	100	2h50m55.305319999s

[didier-wenzek]

This is really nice proposal! I encourage you to finalize it.

[didier-wenzek]

That commit 202b0a6 seems to have been included by accident.

[didier-wenzek]

Have you considered to fully deprecate these test helpers - or, better, to move your improved implementation there? What are the blockers, if any?

The former tedge_mqtt_ext::test_helpers are still used to test the agent and c8y_mapper_ext::availability. Since the purpose is the same it would be good to have a single abstraction.

[Bravo555]

Have you considered to fully deprecate these test helpers - or, better, to move your improved implementation there?

Indeed the new assertions could probably be moved in and replace the current ones in tedge_mqtt_ext, but there's still quite a few usages of the old asserts in the agent tests and I'd like to avoid having this PR becoming too big and replacing everything in one go. Asserts in the agent tests could be changed in a follow-up PR.

The former tedge_mqtt_ext::test_helpers are still used to test the agent and c8y_mapper_ext::availability

Indeed availability module still uses old asserts and skips messages by number. Initially I thought to leave it as is, as the availability module tests don't test the entire mapper but only the availability actor, so it's possible that the availability messages won't change that much and skipped messages are sent by the same actor thus they're not unrelated noise from other actors, so skipping them is meaningful, so i though that maybe the current asserts can stay, but you changed my mind and I'll change those as well because:

• as you say, it's better not to duplicate abstractions, but build one good abstraction and use it everywhere
• the skips often have the form mqtt.skip(1) // 117, i.e. we're expecting the skipped message to be 117 but we're not checking that, which we probably should

So I'll replace these skips with the proper asserts or ignore method from f64978f as required.

[didier-wenzek]

This is not clear. What is done? Why?

This might be a good candidate for a new test helper

Suggested change

	// ignore all messages up to this point
	mqtt.recv_short().await;
	mqtt.messages.lock().unwrap().clear();
	mqtt.ignore_messages_received_so_far();

[Bravo555]

fixed in f64978f

Good catch, indeed that was something I replaced too quickly and didn't realize the test was doing too much - instead of ignoring all messages, we should only ignore any received 114 messages for the child device topic to not mistake it for a 114 message that should be produced when sending command metadata message later on in the test.

[didier-wenzek]

Approved. Thank you. This is great improvement and refactoring c8y mapper tests will be less error prone.

[albinsuresh]

Great enhancement. It was nice to see the seamless transition in all existing tests.

[albinsuresh]

Why not move these test helpers to tedge_mqtt_ext module itself as suggested here, so that these enhancements are available to agent and other components as well?

I wouldn't remove the existing helpers, but just add these to a separate module.

[Bravo555]

Moved into tedge_mqtt_ext::test_helpers::TestMqttBox in 07069fc

I've been putting it off for some reason, but indeed it makes sense and would have needed to be done sooner or later.

[albinsuresh]

Could we use a different parameter name than Box to disambiguate it from the std::boxed::Box type?

[Bravo555]

fixed in 07069fc