Fix the flaky alarms tests #1765
Conversation
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| @@ -1274,6 +1244,7 @@ fn extract_child_id(in_topic: &str, expected_child_id: Option<String>) { | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| #[serial] | |||
There was a problem hiding this comment.
Why do we need to have all these tests serial?
They behave correctly even without serial when both c8y_mapper_alarm_empty_payload and c8y_mapper_child_alarm_empty_payload are ignored.
There was a problem hiding this comment.
Yes, that's right. But, I observed that the test c8y-converter needs the broker, to be on the safer side I made them run serially.
| "tedge/alarms/major/temperature_alarm", | ||
| "tedge/alarms/major/temperature_alarm/external_sensor", |
There was a problem hiding this comment.
Why this instance of tedge/alarms/major/temperature_alarm is replaced by tedge/alarms/major/temperature_alarm/external_sensor while the others (above) are replaced by tedge/alarms/major/custom_temperature_alarm"?
There was a problem hiding this comment.
Because there was a typo in the topic, the alarm that was sent on "tedge/alarms/major/temperature_alarm/external_sensor had to be cleared sending an empty message on the same topic.
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
| while let Ok(Some(msg)) = messages.next().with_timeout(TEST_TIMEOUT_MS).await { | ||
| assert_json_include!(actual:serde_json::from_str::<serde_json::Value>(&msg).unwrap(), expected:expected_msg); | ||
| } | ||
| let expected_msg = r#"{"severity":"MAJOR","type":"custom_temperature_alarm","time":"2023-01-25T18:41:14.776170774Z","text":"Temperature high","customFragment":{"nested":{"value":"extra info"}}}"#; |
There was a problem hiding this comment.
I'm assuming that you converted this to raw string to use the assert_received_all_expected function that expects string. You could have used the to_string() function on the earlier json::Value as well, right? So that you get a string without affecting the code readability. It doesn't matter in this specific case as the JSON was not formatted properly in the first place.
There was a problem hiding this comment.
Yes, thats right. I felt converting from json to string is not necessary if a raw string is used directly.
There was a problem hiding this comment.
Using json! is just for better readability, especially with multi-level JSON structs. More of a good to have feature than a must.
|
|
||
| // Expect converted temperature alarm message | ||
| mqtt_tests::assert_received_all_expected(&mut messages, TEST_TIMEOUT_MS, &[expected_msg]).await; |
There was a problem hiding this comment.
This is still problematic as an assertion failure will prevent the cleanup logic below(clearing alarms) from executing. Not just in this test, but all alarm tests relying on this cleanup behaviour suffers from the same as you discovered yesterday.
There was a problem hiding this comment.
What I observed is that these tests run in serial and when the next test starts it will create a new broker instance, which will be a clean start and no persisted messages.
There was a problem hiding this comment.
So, we don't even need that "alarm cleanup" step in all these tests, huh? Fine then. You can remove those cleanup steps if it's useless anyway. But, up-to you.
|
This PR has to be rebased to get the fix for test report generation. |
PradeepKiruvale
force-pushed
the
flacky-alarm-tet
branch
from
8ffdae7
to
0234c42
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| r#"{ "text": "Temperature high","time":"2023-01-25T18:41:14.776170774Z","customFragment": {"nested":{"value": "extra info"}} }"#, | ||
| mqtt_channel::QoS::AtLeastOnce, | ||
| true, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let expected_msg = json!({"severity":"MAJOR","type":"temperature_alarm","time":"2023-01-25T18:41:14.776170774Z","text":"Temperature high","customFragment":{"nested":{"value":"extra info"}}}); | ||
|
|
||
| while let Ok(Some(msg)) = messages.next().with_timeout(TEST_TIMEOUT_MS).await { |
There was a problem hiding this comment.
So the sporadically test failure is fixed by replacing this block by mqtt_tests::assert_received_all_expected? It looks all other tests have this replacement.
I am just curious, because other locations than alarm also use this while let block for testing.
There was a problem hiding this comment.
Yup, using mqtt_tests::assert_received_all_expected fixed the issue. Before I was trying to assert the json objects and was relying on the timeout error when it does not receive. But, the call with_timeout does not return an error, and because of this, the tests were always passing.
I did check at least two other places where while let is being used, felt it's okay.
Robot Results
Passed Tests
|
| while let Ok(Some(msg)) = messages.next().with_timeout(TEST_TIMEOUT_MS).await { | ||
| assert_json_include!(actual:serde_json::from_str::<serde_json::Value>(&msg).unwrap(), expected:expected_msg); | ||
| } | ||
| let expected_msg = r#"{"severity":"MAJOR","type":"custom_temperature_alarm","time":"2023-01-25T18:41:14.776170774Z","text":"Temperature high","customFragment":{"nested":{"value":"extra info"}}}"#; |
There was a problem hiding this comment.
Using json! is just for better readability, especially with multi-level JSON structs. More of a good to have feature than a must.
|
|
||
| // Expect converted temperature alarm message | ||
| mqtt_tests::assert_received_all_expected(&mut messages, TEST_TIMEOUT_MS, &[expected_msg]).await; |
There was a problem hiding this comment.
So, we don't even need that "alarm cleanup" step in all these tests, huh? Fine then. You can remove those cleanup steps if it's useless anyway. But, up-to you.
Proposed changes
This PR fixes the flaky alarm tests
serialin the way the results validatedTypes of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments