Skip to content

Harden TestClientWithRequestTimeout#39168

Merged
crosbymichael merged 1 commit intomoby:masterfrom
thaJeztah:harden_TestClientWithRequestTimeout
Jul 12, 2019
Merged

Harden TestClientWithRequestTimeout#39168
crosbymichael merged 1 commit intomoby:masterfrom
thaJeztah:harden_TestClientWithRequestTimeout

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented May 1, 2019 •
edited
Loading

splitting this off from #39100 per discussion on #39100 (comment)
addresses #38587 (first reported failure)

DeadlineExceeded now implements a TimeOut() function,
since golang/go@dc4427f

Check for this as well, to prevent possibly incorrect failures;

00:16:41 --- FAIL: TestClientWithRequestTimeout (0.00s)
00:16:41     client_test.go:259: assertion failed:
00:16:41         --- context.DeadlineExceeded
00:16:41         +++ err
00:16:41         :
00:16:41         	-: context.deadlineExceededError{}
00:16:41         	+: &net.OpError{Op: "dial", Net: "tcp", Addr: s"127.0.0.1:49294", Err: &poll.TimeoutError{}}
00:16:41

- How to verify it

make TESTDIRS='github.com/docker/docker/pkg/plugins' TESTFLAGS='-test.run ^TestClientWithRequestTimeout$' test-unit

ok  	github.com/docker/docker/pkg/plugins	0.009s	coverage: 6.4% of statements

@thaJeztah thaJeztah force-pushed the harden_TestClientWithRequestTimeout branch from f96f9dc to 0fdcb85 Compare May 1, 2019 23:43
@thaJeztah thaJeztah mentioned this pull request May 1, 2019
Merged
@codecov
Copy link

codecov bot commented May 2, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@c7d1908). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39168   +/-   ##
=========================================
  Coverage          ?   37.01%           
=========================================
  Files             ?      612           
  Lines             ?    45655           
  Branches          ?        0           
=========================================
  Hits              ?    16898           
  Misses            ?    26468           
  Partials          ?     2289

@thaJeztah
Copy link
Member Author

thaJeztah commented May 2, 2019

@jhowardmsft @cpuguy83 ptal

cpuguy83
cpuguy83 reviewed May 6, 2019
View reviewed changes
pkg/plugins/client_test.go Outdated
Copy link
Member

@cpuguy83 cpuguy83 May 6, 2019

Choose a reason for hiding this comment

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

Why not do something like, define the error interface locally:

type timeoutError interface {
  Timeout() bool
}

And type check against that?

@cpuguy83
Copy link
Member

cpuguy83 commented Jun 8, 2019

Ping

@thaJeztah thaJeztah force-pushed the harden_TestClientWithRequestTimeout branch from 0fdcb85 to 35fd1e1 Compare June 9, 2019 11:10
@thaJeztah
Copy link
Member Author

thaJeztah commented Jun 9, 2019

@cpuguy83 Updated; hope this is what you meant 🤗

@cpuguy83
Copy link
Member

cpuguy83 commented Jun 9, 2019

Not quite, I was thinking a single assertion to a timeout error since they all should implement that.
Then assert that Timeout() is true.

@thaJeztah
Copy link
Member Author

thaJeztah commented Jun 9, 2019

Ahm, right. I wasn't sure if it was part of the contract for these errors (I saw the Timeout was added later on)

@thaJeztah thaJeztah mentioned this pull request Jul 12, 2019
Merged
@thaJeztah thaJeztah force-pushed the harden_TestClientWithRequestTimeout branch from 35fd1e1 to 0b1fbdd Compare July 12, 2019 10:28
@thaJeztah
Harden TestClientWithRequestTimeout
c7816c5 
DeadlineExceeded now implements a TimeOut() function,
since golang/go@dc4427f

Check for this interface, to prevent possibly incorrect failures;

```
00:16:41 --- FAIL: TestClientWithRequestTimeout (0.00s)
00:16:41     client_test.go:259: assertion failed:
00:16:41         --- context.DeadlineExceeded
00:16:41         +++ err
00:16:41         :
00:16:41         	-: context.deadlineExceededError{}
00:16:41         	+: &net.OpError{Op: "dial", Net: "tcp", Addr: s"127.0.0.1:49294", Err: &poll.TimeoutError{}}
00:16:41
```

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the harden_TestClientWithRequestTimeout branch from 0b1fbdd to c7816c5 Compare July 12, 2019 10:32
@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 12, 2019

Test passed;

10:51:57 ok  	github.com/docker/docker/pkg/plugins	33.612s	coverage: 76.8% of statements

@thaJeztah thaJeztah mentioned this pull request Jul 12, 2019
Merged
cpuguy83
cpuguy83 approved these changes Jul 12, 2019
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 requested a review from vdemeester July 12, 2019 17:24
@crosbymichael
Copy link
Contributor

crosbymichael commented Jul 12, 2019

LGTM

@crosbymichael crosbymichael merged commit 6913656 into moby:master Jul 12, 2019
@thaJeztah thaJeztah deleted the harden_TestClientWithRequestTimeout branch July 12, 2019 17:38
This was referenced Aug 9, 2019
Merged
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 14, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Sep 3, 2019
Closed
This was referenced Oct 28, 2019
Merged
Open
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@MHBauer MHBauer

Labels

area/testing process/cherry-picked status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah @cpuguy83 @crosbymichael @GordonTheTurtle @MHBauer