Add unit test of retry support

[rafiramadhana]

Issue: #91

[rafiramadhana]

TODO:

• Add unit test of WithMaxRetries
• Add unit test of WithRetryDelay

[gavv]

Thanks for PR. The approach looks good.

[gavv]

FYI: I've created discord chat for httpexpect. It can be used when we need more interactive discussion.

[rafiramadhana]

ok got it @gavv

this PR is ready now

[gavv]

Thanks, tests look good.

Can we make a few small improvements?

1. For each policy, it'd be nice to cover the case when there is no network and http error.

2. It makes sense to cover POST requests with body, and check that every time request is passed to client, it contains expected body. Because httpexpect has special logic to achieve this, and ideally we should explicitly test it.

   We could cover them separately or just change all requests to POST, up to you.

3. I have an idea how we can better test retry delays. We can add a new unexported field to httpexpect.Request:

   sleepFn func(d Duration)
   

   and modify newRequest to set this field to time.Sleep. Request.retryRequest() should use this function instead of time.Sleep. And in retry delay tests we can override this function with our mock, which will record calls made to it.

   (BTW, we have an issue related to this sleep call: Sleep with context #163; after we fix it, we'll have to update this mock as well, but that is not a problem I think)

[rafiramadhana]

For each policy, it'd be nice to cover the case when there is no network and http error.

ok will do the update

It makes sense to cover POST requests with body, and check that every time request is passed to client, it contains expected body. Because httpexpect has special logic to achieve this, and ideally we should explicitly test it.

i see. agree. btw could you point out where (in which file) the special logic is?

I have an idea how we can better test retry delays. We can add a new unexported field to httpexpect.Request:

i see. i'm ok with that. will do the update too

[rafiramadhana]

@gavv btw for the sleepFn, should we include something like .WithSleepFn too? or we can just do request.sleepFn = func (d time.Duration) { ... }

[rafiramadhana]

(BTW, we have an issue related to this sleep call: #163; after we fix it, we'll have to update this mock as well, but that is not a problem I think)

CMIIW i think the update more into adding test related to context cancellation isn't it?

if so, i think the adjustment will not be that big

[gavv]

@gavv btw for the sleepFn, should we include something like .WithSleepFn too?

Nope. Better let's keep this thing internal, so that only tests can override sleep function. I think this is too low-level detail to be exposed to user.

or we can just do request.sleepFn = func (d time.Duration) { ... }

Yep.

CMIIW i think the update more into adding test related to context cancellation isn't it?

#163 will change the way how we sleep from time.Sleep to something like time.After. So the signature of sleepFn will change and retry tests should be adjusted. But yes, this will be a small change.

[gavv]

btw could you point out where (in which file) the special logic is?

At the beginning of retryRequest we wrap Body with bodyWrapper. Then, before each retry, we call Rewind method to make the body readable again.

[rafiramadhana]

got it

btw perhaps i will update this PR next week

[rafiramadhana]

hi @gavv ,

after implementing sleepFn in this commit (e8ea1ca)

i keep getting this error when running the tests

$ make test

--- FAIL: TestExpectBuilders (0.01s)
    expect_test.go:81: 
        	Error Trace:	expect_test.go:81
        	Error:      	Not equal: 
        	            	expected: &httpexpect.Request{config:httpexpect.Config{TestName:"", BaseURL:"", RequestFactory:httpexpect.DefaultRequestFactory{}, Client:(*httpexpect.mockClient)(0xc0000d46e0), WebsocketDialer:(*websocket.Dialer)(0xc00022e980), Context:context.Context(nil), Reporter:(*httpexpect.AssertReporter)(0xc0005873a8), Formatter:(*httpexpect.DefaultFormatter)(0xc0003bc980), AssertionHandler:(*httpexpect.DefaultAssertionHandler)(0xc0003bfa40), Printers:[]httpexpect.Printer(nil), Environment:(*httpexpect.Environment)(nil)}, chain:(*httpexpect.chain)(0xc00022ec00), redirectPolicy:0, maxRedirects:-1, retryPolicy:2, maxRetries:0, minRetryDelay:50000000, maxRetryDelay:5000000000, sleepFn:(func(time.Duration))(0x467700), timeout:0, httpReq:(*http.Request)(0xc0003c0000), path:"/url", query:url.Values(nil), form:url.Values(nil), formbuf:(*bytes.Buffer)(nil), multipart:(*multipart.Writer)(nil), bodySetter:"", typeSetter:"", forceType:false, wsUpgrade:false, transforms:[]func(*http.Request)(nil), matchers:[]func(*httpexpect.Response)(nil)}
        	            	actual  : &httpexpect.Request{config:httpexpect.Config{TestName:"", BaseURL:"", RequestFactory:httpexpect.DefaultRequestFactory{}, Client:(*httpexpect.mockClient)(0xc0000d46e0), WebsocketDialer:(*websocket.Dialer)(0xc00022e980), Context:context.Context(nil), Reporter:(*httpexpect.AssertReporter)(0xc0005873a8), Formatter:(*httpexpect.DefaultFormatter)(0xc0003bc980), AssertionHandler:(*httpexpect.DefaultAssertionHandler)(0xc0003bfa40), Printers:[]httpexpect.Printer(nil), Environment:(*httpexpect.Environment)(nil)}, chain:(*httpexpect.chain)(0xc00022ec80), redirectPolicy:0, maxRedirects:-1, retryPolicy:2, maxRetries:0, minRetryDelay:50000000, maxRetryDelay:5000000000, sleepFn:(func(time.Duration))(0x467700), timeout:0, httpReq:(*http.Request)(0xc0003c0100), path:"/url", query:url.Values(nil), form:url.Values(nil), formbuf:(*bytes.Buffer)(nil), multipart:(*multipart.Writer)(nil), bodySetter:"", typeSetter:"", forceType:false, wsUpgrade:false, transforms:[]func(*http.Request)(nil), matchers:[]func(*httpexpect.Response)(nil)}
        	            	
        	            	Diff:
        	Test:       	TestExpectBuilders
FAIL
exit status 1
FAIL	github.com/gavv/httpexpect/v2	6.967s
make: *** [Makefile:26: test] Error 1

i am clueless about this assertion error

mind to help? thank you

[gavv]

Good catch!

It's interesting. We see accidental combination of several factors:

• TestExpectBuilders had a bug: the last line should check for equality with r2, not r1

• TestExpectBuilders, as well as some other tests, had another bug: they used incorrectly assertion for comparing pointers; this test should use assert.Same, which succeeds if two pointers are equal; but it instead used assert.Equal, which succeeds if two pointers points to values which are equal

• because of second bug, this first bug remained unidentified: pointers were different, but the values to which they point were equal (both were mostly empty requests with same method and url), and tests passed

• after you added func member to request, the first bug showed itself; it happened because testify can't compare func fields (due to limitations of Go) and thus it considered the two requests non-equal

The proper fix is the following:

• replace r1 with r2 in the last line
• use assert.Same instead of assert.Equal in this test and everywhere else where we intend to compare pointers

I've implemented both fixes in 3358138 and pushed to master. Please rebase your branch on master and the bug should go away.

[rafiramadhana]

@gavv got it

will update by this week

including the sleepFn implementation in unit test

[coveralls]

Coverage increased (+0.6%) to 93.047% when pulling 1d97cd5 on neverbeenthisweeb:91-retry-unit-test into cd6431e on gavv:master.

[rafiramadhana]

@gavv i've added the sleepFn to the unit test

please LMK if there is anything

thank you

[gavv]

Great!

One last small thing: I think in all retry tests we can set sleepFn to no-op, so that there were no unnecessary delays at all, right?

[rafiramadhana]

got it will do the update

[rafiramadhana]

updated 1d97cd5

[gavv]

Thanks! Impressive work!