chore: improve diag-plugins and add system tests#3682
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
albinsuresh
left a comment
There was a problem hiding this comment.
The changes look fine.
But, we'll have to extend the tedge_diag_collect test to do deeper validations beyond the presence of the output.log files for each plugin, bbut also validate what's collected by the factory plugins as well, so that changes like these can be easily validated. We probably don't need to validate each and every line of the collected logs, but some basic validation like the presence of a non-empty file with the expected starting line or something like that would suffice. For static files like tedge.toml, they can be compared for equality as well.
I would go to create another robot file to test the predefined plugins. I think what missing is, the test coverage of the predefined plugins. Since we ship them officially, I understand we need to test and guarantee them. By the way, the general existence of thin-edge.io/crates/core/tedge/src/cli/diag/collect.rs Lines 288 to 289 in 237d8ec |
09d5ee5 to
2cd9a2d
Compare
Robot Results
|
albinsuresh
left a comment
There was a problem hiding this comment.
Happy to approve with some test coverage improvements.
| Custom Suite Setup | ||
| Setup | ||
| Execute Command mkdir -p /results | ||
| Start Service tedge-mapper-collectd |
There was a problem hiding this comment.
Is there possibly a race here? Are the tests assuming the diagnostic collection happens after tedge-mapper-collectd publishes something?
There was a problem hiding this comment.
Nope, tedge-mapper-collectd doesn't have to publish something. As long as some content is logged in journalctl, it's fine. So even this line, indicating the service is starting, is enough.
Jun 16 16:01:26 c2e84027fd54 systemd[1]: Starting tedge-mapper-collectd.service - tedge-mapper-collectd converts Thin Edge JSON measurements to Cumulocity JSON format....
| File Size Is Not Zero ${log_name} | ||
| END | ||
|
|
||
| Log Should Contain tedge-agent.log Starting tedge-agent.service |
There was a problem hiding this comment.
The earlier File Size Is Not Zero check is redundant now, as we're checking the content itself here.
There was a problem hiding this comment.
Though one advantage to checking the file size is zero is that it would produce a more meaningful error if the file is empty (e.g. "File is empty" rather than "File does not contain "xxxx")
There was a problem hiding this comment.
Though one advantage to checking the file size is zero is that it would produce a more meaningful error if the file is empty (e.g. "File is empty" rather than "File does not contain "xxxx")
Exactly this is the reason why I kept it. Also, it just calls test -s xxx, so it should be a light job and finish quickly.
There was a problem hiding this comment.
In that case I'd have done the File Size Is Not Zero from the Log Should Contain function itself. But anyway, minor thing.
| if command -V journalctl >/dev/null 2>&1; then | ||
| mosquitto_journal | ||
| mosquitto_log | ||
| else | ||
| echo "mosquitto not found" >&2 | ||
| mosquitto_log | ||
| fi |
There was a problem hiding this comment.
We'll need to change this logic to check both locations, journald logs and the default location of the mosquitto file log, as you can configure mosquitto to log to file instead of syslog (journald) even if you have journalctl installed.
There was a problem hiding this comment.
So just move the "mosquitto_log" out of the journalctl if/else condition.
5a88ac5 to
a9fcb3b
Compare
* 01_tedge.sh collects tedge.toml, tedge config list output, and tedge-mapper-collectd log * 07_mosquitto.sh collects mosquitto.log and journal log * Rename CONFIG_DIR to TEDGE_CONFIG_DIR in diag plugins * Add system tests for all predefined diag plugins Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
a9fcb3b to
a7b70ba
Compare
Proposed changes
According to the review 1.-4. points from #3646 (comment)
01_tedge.shcollects tedge.toml and tedge config list output (->1.)01_tedge.shcollects also collectd mapper log (->2.)07_mosquitto.shcollects either journalctl output and mosquitto.log (->4.)In addition, add system tests for all predefined diag plugins.
Types of changes
Paste Link to the issue
#3646
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments