https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422014-09-06T00:01:22ZArvadosArvados - Bug #3828: [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like thathttps://dev.arvados.org/issues/3828?journal_id=146692014-09-06T00:01:22ZWard Vandewegeward@curii.com
<ul><li><strong>Subject</strong> changed from <i>[Keepproxy] insufficient error checking on "defer listener.Close()" lines, cf. jenkins run 846</i> to <i>[Keepproxy] insufficient error checking on "defer listener.Close()" lines, cf. jenkins run 846 where the keepproxy tests failed like that</i></li></ul> Arvados - Bug #3828: [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like thathttps://dev.arvados.org/issues/3828?journal_id=146742014-09-08T09:20:15ZWard Vandewegeward@curii.com
<ul><li><strong>Subject</strong> changed from <i>[Keepproxy] insufficient error checking on "defer listener.Close()" lines, cf. jenkins run 846 where the keepproxy tests failed like that</i> to <i>[Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like that</i></li></ul> Arvados - Bug #3828: [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like thathttps://dev.arvados.org/issues/3828?journal_id=160212014-10-06T18:29:52ZTom Cleggtom@curii.com
<ul></ul><pre>
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
</pre> Arvados - Bug #3828: [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like thathttps://dev.arvados.org/issues/3828?journal_id=160322014-10-06T20:07:34ZTom Cleggtom@curii.com
<ul><li><strong>Category</strong> set to <i>Tests</i></li><li><strong>Assigned To</strong> set to <i>Tom Clegg</i></li><li><strong>Target version</strong> set to <i>2014-10-08 sprint</i></li></ul> Arvados - Bug #3828: [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like thathttps://dev.arvados.org/issues/3828?journal_id=160372014-10-06T20:35:39ZTim Piercetwp@curoverse.com
<ul></ul><p>review @ commit <a class="changeset" title="3828: Wait for listener to start before connecting to it. Fix test panic in listener.Close() when..." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/d9166dfa6e46321e654f13fac59e8d10362ad169">d9166dfa</a></p>
<p>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.</p>
<p>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.</p> Arvados - Bug #3828: [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like thathttps://dev.arvados.org/issues/3828?journal_id=160452014-10-06T22:00:01ZTom Cleggtom@curii.com
<ul></ul><p>Tim Pierce wrote:</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>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. :)</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>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.</p>
<p>(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...)</p> Arvados - Bug #3828: [Keepproxy] insufficient error checking on "defer listener.Close()" lines in the tests, cf. jenkins run 846 where the keepproxy tests failed like thathttps://dev.arvados.org/issues/3828?journal_id=160462014-10-06T22:05:08ZAnonymous
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li></ul><p>Applied in changeset arvados|commit:792b42b0f28f1b2595be071d0683c4872af0a6db.</p>