clients/horizonclient: allow Fund on other networks and rely on server to indicate support #3891
Conversation
…r to indicate support
leighmcculloch
requested review from
ire-and-curses and
a team
and removed request for
ire-and-curses
| ).ReturnString(404, friendbotFundResponse) | ||
|
|
||
| _, err := client.Fund("GBLPP2W3X3PJQXYMC7EFWM5G2QCZL7HTCTFNMONS4ITGAYJ3GNNZIQ4V") | ||
| assert.EqualError(t, err, "funding is only available on test networks and may not supported by https://localhost/: horizon error: \"Resource Missing\" - check horizon.Error.Problem for more information") |
There was a problem hiding this comment.
This is more of a philosophical discussion, but I've been meaning to ask this for a while: why do we test for specific error messages like this rather than just assert.Error, or maybe even a regex match for a substring on the error? I've read that the latter is considered an anti-pattern, but don't fully understand why.
My rationale against it:
- awkward hard-coding of errors: we can't use constants, and so we have long strings here
- increased test verbosity without (in my opinion) added value. For example, the message has a typo (see above), but this test wouldn't have caught that.
- discourages changing error messages that may increase clarity or information density, since there are many places to edit it
Again, I get that this is more of a philosophical best-practices discussion (and one that shouldn't impede merging this PR ✔️), but this seemed like an isolated-enough PR to get myself some clarity on that.
There was a problem hiding this comment.
I think the distinction here is that this error string, that is wrapping the not found error in, is just an annotation or a hint, like a comment about the error. The error is still the Horizon not found error in the propoer horizon client error object. Because of this I don't it is worth adding a new error type for the annotation.
In regards to more generally, the reason that regexing isn't preferred is it is brittle. The preferred approach for error matching is to use errors.Is or errors.As, which works great with custom error types that wrap an error, or error constants that don't wrap another error.
In this specific case we can't use a constant for the error, otherwise we'd lose the underlying horizon not found error, which I think is actually the most important error in this scenario. In this specific case I don't think it is worth defining an error type to wrap the not found error.
There was a problem hiding this comment.
I misunderstood your question.
why do we test for specific error messages like this rather than just
assert.Error
assert.Error would tell us that any error occurred. Any error could occur for a variety of other reasons and we wouldn't necessarily be testing the exact scenario we intend.
assert.EqualError is maybe a little stricter than we need, but it works?
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Allow the
Fundfunction to submit a fund request to any horizon instance, not just the instance defined byhorizonclient.DefaultTestNetClient, and rely on the horizon instance responding with a 404 as an indicator that funding is not supported.Why
The only way to construct a
horizonclient.Clientthat supports theFundoperation is to use and mutate thehorizonclient.DefaultTestNetClient. This is unnecessarily restrictive because a developer may wish to use theFundoperation with a non-default client that they have constructed themselves, or they may wish to use theFundoperation with their own private or standalone test network.Known limitations
N/A