Bug #3828

[Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like that

Added by Ward Vandewege almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Tests
Target version:
Start date:
10/06/2014
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
0.5

Subtasks

Task #4119: Review 3828-keepproxy-raceResolvedTim Pierce

Associated revisions

Revision 792b42b0
Added by Tom Clegg almost 5 years ago

Merge branch '3828-keepproxy-race' closes #3828

Revision 3147edd0 (diff)
Added by Tom Clegg over 4 years ago

Reset listener=nil before running main() from test cases, so
waitForListener() does not get confused by listener!=nil left over
from previous tests. Fixes intermittent test failures.

refs #3828

History

#1 Updated by Ward Vandewege almost 5 years ago

  • Subject changed from [Keepproxy] insufficient error checking on "defer listener.Close()" lines, cf. jenkins run 846 to [Keepproxy] insufficient error checking on "defer listener.Close()" lines, cf. jenkins run 846 where the keepproxy tests failed like that

#2 Updated by Ward Vandewege almost 5 years ago

  • Subject changed from [Keepproxy] insufficient error checking on "defer listener.Close()" lines, cf. jenkins run 846 where the keepproxy tests failed like that to [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like that

#3 Updated by Tom Clegg almost 5 years ago

2014/10/06 14:10:57 TestGetDisabled start
2014/10/06 14:10:57 keepclient created

----------------------------------------------------------------------
PANIC: /tmp/tmp.O0vXh97Iqx/src/git.curoverse.com/arvados.git/services/keepproxy/keepproxy_test.go:239: ServerRequiredSuite.TestGetDisabled

... Panic: runtime error: invalid memory address or nil pointer dereference (PC=0x414086)

/usr/local/go/src/pkg/runtime/panic.c:248
  in panic
/usr/local/go/src/pkg/runtime/panic.c:482
  in panicstring
/usr/local/go/src/pkg/runtime/os_linux.c:234
  in sigpanic
/tmp/tmp.O0vXh97Iqx/src/git.curoverse.com/arvados.git/services/keepproxy/keepproxy_test.go:243
  in ServerRequiredSuite.TestGetDisabled

#4 Updated by Tom Clegg almost 5 years ago

  • Category set to Tests
  • Assigned To set to Tom Clegg
  • Target version set to 2014-10-08 sprint

#5 Updated by Tim Pierce almost 5 years ago

review @ commit d9166dfa

There may be a clever way to do this more reliably without exposing ugly test harness stuff, but it's not obvious. For our purposes, this seems like a fine improvement over the status quo, so LGTM.

Going forward, I think it would help Keepstore and Keepproxy (and probably Data Manager) development to have comprehensive mock/fake interfaces for Keepstore and apiserver, so any of these packages' test suites can just instantiate a FakeKeepstore or FakeApiServer that runs in a thread and responds to requests with canned responses. The kind of integration test we are doing here is useful, but it would be nice to test as much as possible in a more deterministic environment.

#6 Updated by Tom Clegg almost 5 years ago

Tim Pierce wrote:

There may be a clever way to do this more reliably without exposing ugly test harness stuff, but it's not obvious. For our purposes, this seems like a fine improvement over the status quo, so LGTM.

Surely it's possible to put hooks into http.Serve() using testing and/or gomock, but for now, "server isn't listening within 1 second" is arguably a real bug anyway. :)

Going forward, I think it would help Keepstore and Keepproxy (and probably Data Manager) development to have comprehensive mock/fake interfaces for Keepstore and apiserver, so any of these packages' test suites can just instantiate a FakeKeepstore or FakeApiServer that runs in a thread and responds to requests with canned responses. The kind of integration test we are doing here is useful, but it would be nice to test as much as possible in a more deterministic environment.

I agree, mocking the back-end services will let us test more stuff -- like appropriate behavior when back-end services misbehave -- and give us a much quicker test suite.

(Kind of funny that the Keepproxy-under-test itself, rather than the run_test_server stuff, is the source of this server-not-ready race...)

#7 Updated by Anonymous almost 5 years ago

  • Status changed from New to Resolved

Applied in changeset arvados|commit:792b42b0f28f1b2595be071d0683c4872af0a6db.

Also available in: Atom PDF