-
Notifications
You must be signed in to change notification settings - Fork 968
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
test(gateway): slight refactor of some presentation and add some endpoint registration tests #2939
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2939 +/- ##
==========================================
+ Coverage 51.00% 51.69% +0.69%
==========================================
Files 177 178 +1
Lines 11147 11196 +49
==========================================
+ Hits 5685 5788 +103
+ Misses 4959 4909 -50
+ Partials 503 499 -4 ☔ 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.
Looks good, glad we will have something on both gateway and rpc for health/readiness.
Talking with Jordan Oroshiba, health should be an endpoint that determines if the node is "healthy" ofc, but idrk what metrics we would use to determine this... hmm
In our case it should be checking for deadlocks/stalls. E.g it would ask Syncer and DASer for their state and if they don't respond or change their state in a timeout then we make a conclusion that things aren't healthy. This is btw also why Ready check is not health check @ramin |
goals: readability, test coverage
Appreciate that the gateway is v unimportant but is exists. I was starting at the top just looking for places to give a clean up and add some unit test coverage and found this.
Mostly trying to make it cleaner to read, then added some tests, also wanted to add a
/status/health
endpoint just for ease, i would want this as a node operator if i was running with--gateway
for whatever reasonAlso took liberty to take some error handling code out of
writeError
that would never execute. In trying to write a test to trigger the json.Marshal error case, i realized that as err.Error() always returns a string it could never trigger json.Unmarshal to fail without panic before even getting there, so removed that code path.i might revisit this and give it another refactor at some point soon, unless we're planning to remove this completely?
on to next area