-
-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update to 8.0 for e2e tests #592
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #592 +/- ##
==========================================
+ Coverage 63.03% 63.14% +0.11%
==========================================
Files 279 279
Lines 14042 14071 +29
Branches 1817 1816 -1
==========================================
+ Hits 8851 8885 +34
+ Misses 4567 4564 -3
+ Partials 624 622 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)
* Use Protobuf.System.Text.Json to properly format dictionaries: https://stackoverflow.com/questions/66300807/protobuf-map-unserialization-issue * Update outbox handling to deal with the new Json Options * FIx E2E test properly getting build info after build is complete
@Enkidu93 - more changes to get E2E tests to pass... |
The options were either add this package or use Newtonsoft (https://stackoverflow.com/questions/66300807/protobuf-map-unserialization-issue). I really don't care one way or the other. A lot of the other changes were to centralize the |
Passing the group Id is so that the stream can have the engineId (which is not in the stream itself but is needed). These changes can be reverted, but it makes more logical sense to me to handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)
src/Machine/src/Serval.Machine.Shared/Serval.Machine.Shared.csproj
line 39 at r2 (raw file):
Previously, johnml1135 (John Lambert) wrote…
The options were either add this package or use Newtonsoft (https://stackoverflow.com/questions/66300807/protobuf-map-unserialization-issue). I really don't care one way or the other. A lot of the other changes were to centralize the
JsonSerializationOptions
that need to be referenced when both serializing and deserializing (https://github.com/Havret/Protobuf.System.Text.Json).
I think I would prefer to use Newtonsoft. It is a better tested and more widely used package and we already have a dependency on it.
src/Machine/src/Serval.Machine.Shared/Services/MessageOutboxDeliveryService.cs
line 107 at r2 (raw file):
Previously, johnml1135 (John Lambert) wrote…
Passing the group Id is so that the stream can have the engineId (which is not in the stream itself but is needed). These changes can be reverted, but it makes more logical sense to me to handle
content
andcontentStream
in different calls. This also corresponds with the serialization no longer being standard, but needing to have the protobuf support added. It made more logical sense to have that at the MessageOutboxService level rather than the ServalPlatformService level.
These look like good improvements.
Previously, ddaspit (Damien Daspit) wrote…
Newtonsoft appears to be in maintenance mode - it was last released in March '23. Moreover, there is no Sync Deserialize support (nor will there be): https://stackoverflow.com/questions/72451669/deserializing-to-asyncenumerable-using-newtonsoft-json. I am inclined to keep with the newer package instead of writing (or copying) a deserializer in for something that will never be rolled out in the main package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)
src/Machine/src/Serval.Machine.Shared/Serval.Machine.Shared.csproj
line 39 at r2 (raw file):
Previously, johnml1135 (John Lambert) wrote…
Newtonsoft appears to be in maintenance mode - it was last released in March '23. Moreover, there is no Sync Deserialize support (nor will there be): https://stackoverflow.com/questions/72451669/deserializing-to-asyncenumerable-using-newtonsoft-json. I am inclined to keep with the newer package instead of writing (or copying) a deserializer in for something that will never be rolled out in the main package.
No async support is an issue, so I'm good with using the newer package.
These fixes are needed to get E2E tests working:
This change is