Skip to content
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

Pass context to Thrift post response callback #465

Merged
merged 3 commits into from
Jul 26, 2016

Conversation

robskillington
Copy link
Contributor

@robskillington robskillington commented Jul 25, 2016

assert.Equal(t, "Call", method)
assert.Equal(t, ctx.Value(key), value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Equal should have expected argument before the value it's testing, so this should be,

assert.Equal(t, value, ctx.Value(key))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test is testing what you're intending for it to test. So right now, you're using the contextFn to create the context that's passed in to the to the test function. So you're basically testing that your test logic creates the context using the function you specified, and that it has the value you set.

What you actually want to test is: if you call SetContextFn (https://godoc.org/github.com/uber/tchannel-go/thrift#Server.SetContextFn) to override the context creation to include an extra value, the context you receive inside of the cb has that same value.

You probably want your contextFn to be used to call SetContextFn on a thrift.Server right after creating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I just wanted to test its the same context being threaded through, don't really mind about where exactly it's created from.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that reqCtx is not being tested -- you could pass nil as a parameter to the context in the Thrift code, and the test would still pass.

The test should make sure that reqCtx is the same context that is returned from the context creation function set by SetContextFn

@prashantv
Copy link
Contributor

The library change looks good, but the test isn't testing the right behaviour.

@robskillington
Copy link
Contributor Author

Updated @prashantv with changes, was a lot easier to test with SetContextFn that I originally had believed.

@robskillington robskillington force-pushed the pass-context-to-response-callback branch from 48ff0ee to ded07bb Compare July 26, 2016 01:53
@prashantv
Copy link
Contributor

lgtm, thanks @robskillington

@prashantv prashantv merged commit 4006710 into dev Jul 26, 2016
@prashantv prashantv deleted the pass-context-to-response-callback branch July 26, 2016 02:29
@prashantv prashantv mentioned this pull request Aug 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants