test: Add reproducer of 4622 (negative connection count)#4855
test: Add reproducer of 4622 (negative connection count)#4855mkleczek wants to merge 3 commits intoPostgREST:mainfrom
Conversation
mkleczek
force-pushed
the
test/4622-reproducer
branch
from
670baba to
c7a8cdb
Compare
| spec :: SpecWith (SpecState, Application) | ||
| spec = describe "Tests using Toxiproxy" $ do | ||
| it "Should return 503 on temporary database server unavailability" $ do | ||
| pendingWith "TODO fix" | ||
| SpecState{specAppState, specToxiProxy} <- getState | ||
|
|
||
| -- make sure there are no open connections | ||
| liftIO $ AppState.flushPool specAppState | ||
|
|
||
| liftBaseDiscard (withDisabled specToxiProxy) $ do | ||
| void $ get "/items?id=eq.5" | ||
| `shouldRespondWith` 503 | ||
|
|
||
| void $ get "/items?id=eq.5" | ||
| `shouldRespondWith` 200 | ||
|
|
||
| liftBaseDiscard (withDisabled specToxiProxy) $ do | ||
| void $ get "/items?id=eq.5" | ||
| `shouldRespondWith` 503 | ||
|
|
||
| it "Must not have negative connection count" $ do | ||
| SpecState{specAppState, specToxiProxy, specMetrics=Metric.MetricsState{poolAvailable}} <- getState | ||
|
|
||
| -- make sure there are no open connections | ||
| liftIO $ AppState.flushPool specAppState | ||
|
|
||
| liftBaseDiscard (withDisabled specToxiProxy) $ do | ||
| replicateM_ 5 $ get "/authors_only?id=eq.5" | ||
| `shouldRespondWith` 503 | ||
|
|
||
| -- TODO https://github.com/PostgREST/postgrest/issues/4622 | ||
| -- change to 0 when fixed | ||
| -- HSpec does not support xfail which should be used instead | ||
| liftIO $ getGauge poolAvailable >>= (`shouldBe` (-5::Int)) . round |
There was a problem hiding this comment.
The benefit of this integration is much more clear now.
| @@ -0,0 +1,347 @@ | |||
| {- | |||
There was a problem hiding this comment.
Regarding #4836 (comment), can't we delete the code we don't use?
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: createProxy
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: createToxic
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: deleteProxy
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: deleteToxic
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getProxies
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getProxy
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getToxic
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getToxics
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getVersion
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: postPopulate
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: postReset
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: updateToxic
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:331:1: withToxic
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:343:1: withProxy
From https://github.com/PostgREST/postgrest/actions/runs/25049698636/job/73374301118?pr=4855
There was a problem hiding this comment.
Regarding #4836 (comment), can't we delete the code we don't use?
postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: createProxy postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: createToxic postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: deleteProxy postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: deleteToxic postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getProxies postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getProxy postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getToxic postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getToxics postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: getVersion postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: postPopulate postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: postReset postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:282:1: updateToxic postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:331:1: withToxic postgrest-15-inplace-observability: test/observability/Toxiproxy.hs:343:1: withProxyFrom https://github.com/PostgREST/postgrest/actions/runs/25049698636/job/73374301118?pr=4855
Yeah... I've left it for now as the code is copied as-is from https://github.com/jpittis/toxiproxy-haskell and I was not sure what to do with it.
Happy to remove unused code, the only thing I am hesitant to delete is withToxic - we do not have these kind of tests yet but I expect to have them in the future.
wolfgangwalther
left a comment
wolfgangwalther
left a comment
There was a problem hiding this comment.
Thinking about whether this should be an hspec or IO test, in light of #4860 (comment) and #4836 (review):
It seems to me like this test would not suffer from the "brittle log checking" problem we currently have with IO tests. Instead we'd check the return of the metrics endpoint, correct?
True - if we had |
mkleczek
force-pushed
the
test/4622-reproducer
branch
from
c7a8cdb to
f546667
Compare
This commit introduces toxiproxy in withTools adding the following helpers: * withToxiproxyServer * withToxiproxyProxy * withToxiproxyPgProxy
mkleczek
force-pushed
the
test/4622-reproducer
branch
from
f546667 to
c02231e
Compare
| -- make sure there are no open connections | ||
| liftIO $ AppState.flushPool specAppState |
There was a problem hiding this comment.
Currently trying to understand this test to get an idea which of these new tests in currently open PRs could just as well be done in pytest and which problems would come up (so far, I have not found any which would depend on brittle log checking, but I'm still looking).
I understand this line to be required because of how the hspec tests work - they keep one AppState for the whole test-suite. In the io tests, this test would get a separate PostgREST instance started, so flushing the pool should not be required, correct?
(everything else in this test seems easily doable via pytest, assuming toxiproxy support was added there, and would not depend unstructured log checking or unreliable timing-based testing)
3 participants
DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.
Stacked on top of #4836
The test can be enabled (ie. the assertion changed to a proper one) in #4672 if this PR is merged into
main.