-
Notifications
You must be signed in to change notification settings - Fork 12
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
Remove entropy from client failures #262
Conversation
d102c1f
to
6ce796f
Compare
6ce796f
to
b7a39d5
Compare
val error = s"Unexpected server response: $body" | ||
RegistryError.ClientFailure(error).asLeft | ||
} | ||
val error = s"Unexpected server response: ${response.status.code}" |
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.
Maybe "Unexpected response code" is a better message?
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.
definitely, not every response is related to server
} | ||
|
||
Http4sRegistryLookup(client).lookup(repositoryRef, schemaKey).map { result => | ||
result should beLeft(ClientFailure(s"Unexpected server response: ${Forbidden.code}")) |
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.
I think you should assert that the string matches the exact string you expect, i.e. check for
"Unexpected server response: 403"
In my experience, in tests it is unhelpful to be too clever with using stringified objects.
Imagine http4s makes a code change in a future release, where they change the toString
method on Forbidden
:
object Forbidden extends Status {
override def toString(): String = "foo"
}
Would your unit test pass or fail if http4s made that change?
I know that sounds like a silly example, but I can remember bugs/incidents caused by exactly this mistake! There was a bug in the old collector, where akka changed how they serialize sensitive headers. We didn't notice that akka had changed their implementation, because our unit tests made the same mistake as the code under test.
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.
Found the collector bug I'm referring to: snowplow/stream-collector#210
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.
I see what you mean, passing tests would trick us if/when toString changes, updating
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.
Nice :)
This PR removes details from client failures and include status code only so that entropy is minimized.
ref: PDP-1642