-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix: Add missing delta payload #2306
fix: Add missing delta payload #2306
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2306 +/- ##
===========================================
+ Coverage 74.05% 74.16% +0.12%
===========================================
Files 259 259
Lines 25808 25808
===========================================
+ Hits 19110 19140 +30
+ Misses 5358 5334 -24
+ Partials 1340 1334 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review 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.
Fix looks good! Just one question about encoding.
{ | ||
"cid": "bafybeiddpjl27ulw2yo4ohup6gr2wob3pwagqw2rbeaxxodv4ljelnu7ve", | ||
"collectionID": int64(1), | ||
"delta": []uint8([]byte{0x16}), |
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.
question: should this be a base64 encoding instead?
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.
yes. Trying to figure something out for this. It is base64 when using CLI and HTTP clients but not when using the go client.
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.
So it's base64 with CLI and HTTP clients because that's how json.Marshal
will convert []uint8
to string
representation. I've added support for []uint8
result assertion when using the CLI and HTTP clients.
@@ -47,47 +47,47 @@ var ( | |||
Name: request.CommitTypeName, | |||
Description: commitDescription, | |||
Fields: gql.Fields{ | |||
"height": &gql.Field{ | |||
request.HeightFieldName: &gql.Field{ |
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.
praise: this is much cleaner
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.
It looks nicer, but now we have lost protection against these changing. I have mixed feelings about this change.
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.
For reference, I was being slow and assumed this was a test :) This change is very welcome 😁
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.
Looks good to me, and thank you for the tests :)
I am a bit uncomfortable about the use of production constants in the expected test results, but not uncomfortable enough to block it :)
087a404
to
cd4cb94
Compare
cd4cb94
to
2479f2e
Compare
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.
Thanks for this fix! LGTM
## Relevant issue(s) Resolves sourcenetwork#2299 ## Description This PR fixes a regression bug and adds integration tests to ensure that future changes don't break the expected query results
Relevant issue(s)
Resolves #2299
Description
This PR fixes a regression bug and adds integration tests to ensure that future changes don't break the expected query results
Tasks
How has this been tested?
make test and manual queries
Specify the platform(s) on which this was tested: