Test: Replace sync_timeline_event! with EventFactory for beacon events in room integration test
#5051
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5051 +/- ##
=======================================
Coverage 85.85% 85.85%
=======================================
Files 325 325
Lines 35937 35937
=======================================
Hits 30855 30855
Misses 5082 5082 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| timeline_events.push( | ||
| f.beacon( | ||
| owned_event_id!("$15139375514XsgmR:localhost"), | ||
| format!("geo:{nth}.9575274619722,12.494122581370175;u={nth}"), |
There was a problem hiding this comment.
This is a bit too stringly typed for my taste.
What do you think about passing in the longitude, latitude and uncertainty as arguments and the string gets built in the beacon() function?
There was a problem hiding this comment.
Yeah it's a good point, I agree that the geo URI could be handled more robustly rather than just passing a string, I'll do that!
There was a problem hiding this comment.
Done in fixup commit dbf6516 + corresponding changes to tests done in fixup commit 4c3957c.
I modified the asserts because the printing of the float latitude and longitude values was "slightly off", most likely due to floating-point representation oddities. An example I saw from one of the tests would probably explain it more succinctly; this code:
println!("value is {}", 12.494122581370175);prints this:
value is 12.494122581370174
Similarly for 10.000000 getting printed as 10 for the other test.
So, I opted to use the format! macro to produced the expected value/LHS of the assertions.
The nicest way to do this could be pretty subjective, so no worries at all about further comments on making it better. I decided to interpolate the float values as they're the potentially problematic ones, and I put the integer value inside the string literal since integers aren't problematic like decimals represented with floating-point.
| format!("geo:{nth}.9575274619722,12.494122581370175;u={nth}"), | ||
| Some(MilliSecondsSinceUnixEpoch(1_636_829_458u32.into())), | ||
| ) | ||
| .event_id(<&EventId>::try_from(format!("$event_for_stream_{nth}").as_str()).unwrap()) |
There was a problem hiding this comment.
Putting the string or the event ID into a variable at the top might be a bit less gnarly.
let event_id: &EventId = <&EventId>::try_from(format!(...));There was a problem hiding this comment.
This is a fair suggestion, I think inlining the entire thing was a bit much, I'll split it up
There was a problem hiding this comment.
Done in fixup commit eb9ae84
| @@ -790,6 +806,16 @@ impl EventFactory { | |||
| self.event(event) | |||
| } | |||
|
|
|||
| /// Create a new `org.matrix.msc3672.beacon` event. | |||
There was a problem hiding this comment.
An example for this method would be nice, though not required.
There was a problem hiding this comment.
Sure thing, I'm happy to make the docstring more useful by adding an example, thanks for the suggestion!
There was a problem hiding this comment.
Done in fixup commit 819ee49
Part of #3716
I think I've done something sensible for adding the ability to put age into the unsigned data of an event, but it's possible I've misunderstood how the fields on
Unsignedare being used.It seemed like fields from different
*Unsignedtypes fromrumawere all put onto the singleUnsignedstruct (ie,transaction_idandrelationsfromRoomMemberUnsigned, andredacted_becausefromRedactedUnsigned), and that some but not all fields on anUnsignedinstance within anEventBuilderwould be made aSomevalue depending on what event type was being created.So, I just stuck another field onto
Unsignedto allowageto be in the unsigned data, even though it's not a field that's available on every*Unsignedtype, imagining it'd be used only for events which actually could haveagein the unsigned data. Hope that makes sense!And apologies for the slightly gnarly
<&EventId>::try_frombit. I would have just used theevent_id!macro, but rust doesn't allow doingevent_id!(format!(...))(ie, trying to run one macro and then pass its output as input to another macro).Signed-off-by: Yousef Moazzam [email protected]