Make transaction-rollback=true the default for test-suite #1663

wolfgangwalther · 2020-11-21T20:32:41Z

Follow-up on #1659:

And then for another PR: Make rollback the default for our spec-tests. This will require quite a bit of cleanup. My goal would be to be able to run the whole test suite multiple times against the same database without recreating the schema.

This is based on the other PR, so the diff is a bit longer than it should be. Still, it's huge. I hope it looks better with "Hide whitespace changes" turned on.

Quick guide on how to look at this:

Everything in src/... is from the other PR. Ignore here.
Changes have been made to test/Main.hs and test/SpecHelper.hs. Removed the schema reloading, re-ordered the tests and enabled transaction-rollback = true. This caused a lot of broken tests.
Everything in test/Feature/... + test/fixtures/... is fixing those tests, so that they don't depend on the result of other tests anymore. As always, every test-case I touched, I also reformatted / refactored. This might increase the diff here, but I hope to be able to make all those test-cases consistently structured down the road. This helps me a lot to understand the tests quickly when reading them. The changes to the tests, that I had to make to account for the rolled back transactions are often quite similar. For every kind of change, I will comment on an example.

When refactoring / reformatting the tests, I did the following:

When possible, replaced code like this:

p <- request ...
liftIO $do
  simpleBody p `shouldBe` ...
  simpleStatus p `shouldBe` ...
... lookup hLocation $ simpleHeaders p
...

with:

request ...
  `shouldRespondWith`
    <body>
    { matchStatus = ...
    , matchHeaders = ... }

Much easier to read, because it follows the same structure that almost all the tests have.

When possible replaced request methodPost ... with the short form post ... (for all http methods). Fewer empty arrays and empty bodies that way.
Formatted the requests around shouldRespondWith with consistent indentation and newlines, like this:

request <method> <uri>
    <headers>
    <body>
  `shouldRespondWith`
    <body>
    <status/headers>

For me, it's much easier to immediately see what's going on, compared to when the shouldRespondWith is sometimes hidden somewhere on the same line as other things.

Changed [str|...json data...|] to [json|...json data...|]. This is essential, because shouldRespondWith will then do a json-comparison and ignore formatting, which sometimes causes trouble. It does also include a check for the application/json content-type by default. I don't think there were any changes of this, in this PR.
Removed obsolete matchContentTypeJson (see last point why, it's included already) and matchStatus = 200 (the default check).

@steve-chavez do you think this formatting / structure for tests is something that we can "formalize" a bit more, even without a linter / style-check that covers for it? Any suggestions for improvement?

test/Feature/AndOrParamsSpec.hs

test/Feature/DeleteSpec.hs

test/Feature/InsertSpec.hs

test/Feature/UpsertSpec.hs

test/Main.hs

test/fixtures/schema.sql

wolfgangwalther · 2020-11-21T21:04:57Z

~~And it seems like stack-test is failing for two tests in CI. postgrest-test-spec-all runs fine on my local machine, so I'll have to see where the difference is.~~ should be fixed now.

And I did not run the tests against the same database without recreation yet. That's something that I want to do as well, I'm pretty sure some more failing tests will come up.

test/Feature/EmbedDisambiguationSpec.hs

steve-chavez · 2020-11-24T22:15:21Z

@wolfgangwalther A rebase here would be good. I'll carefully review this one in case something slips by on the modified tests.

wolfgangwalther · 2020-11-24T23:14:15Z

Rebased.

I'll carefully review this one in case something slips by on the modified tests.

Yes, that would be great, thanks!

If possible, you could also try to run the tests twice against the same database, without re-creating the schema. I haven't been able to do that, yet. And I think the tests about "execute a rpc only once per request" need special attention as well. Not sure right now, if I even touched them, but they for sure need to keep state on the database. So they probably need to clean up as well, not sure they do.

test/Feature/SingularSpec.hs

test/Feature/UpdateSpec.hs

test/Feature/UpsertSpec.hs

steve-chavez · 2020-12-03T01:12:20Z

@wolfgangwalther Finally gave this one a review :) Check my above comments.

I like that persistence in the tests has to be explicit now with Prefer: tx=commit. Also noted that for testing errors we should now use Prefer: tx=commit as well.

If possible, you could also try to run the tests twice against the same database, without re-creating the schema.

Not sure how to do that quickly :(. I just ran postgrest-test-spec and that's working good. The RPC-only-running-once test looked fine when I gave it a review.

wolfgangwalther · 2020-12-03T09:55:02Z

I like that persistence in the tests has to be explicit now with Prefer: tx=commit. Also noted that for testing errors we should now use Prefer: tx=commit as well.

I somehow can't see my comment about this above easily, so I repeat what I wrote there, here:

Not all errors need Prefer: tx=commit. Once you test for "non-persistance after an error" you need it. Most tests for errors do not have the "non-persistance test" part.

If possible, you could also try to run the tests twice against the same database, without re-creating the schema.

Not sure how to do that quickly :(.

Yeah, I wasn't sure about that either. I think I now know how: I will just wrap two calls to cabal v2-test in the same test/with_tmp_db call. That should work. I will let you know what happens.

wolfgangwalther · 2020-12-03T12:04:37Z

So I did this, and it worked:

test/with_tmp_db sh -c 'cabal v2-test -f FailOnWarn --test-show-detail=direct && cabal v2-test -f FailOnWarn --test-show-detail=direct'

Showing another 6 failures. Will fix those in a 2nd commit, so you can review those independently.

wolfgangwalther · 2020-12-03T13:21:44Z

Ok, this should be good to merge now from my perspective.

steve-chavez · 2020-12-03T16:39:22Z

@wolfgangwalther Same here. Looks good to go!

…1663) Change test-suite to use db-tx-rollback-all = true by default

wolfgangwalther commented Nov 21, 2020

View reviewed changes

wolfgangwalther force-pushed the rollback-tests-default branch from 67c0883 to 5b3ec80 Compare November 21, 2020 21:01

wolfgangwalther force-pushed the rollback-tests-default branch from 5b3ec80 to 426a304 Compare November 21, 2020 21:19

wolfgangwalther commented Nov 21, 2020

View reviewed changes

test/Feature/EmbedDisambiguationSpec.hs Show resolved Hide resolved

wolfgangwalther force-pushed the rollback-tests-default branch from 426a304 to 0294c31 Compare November 21, 2020 21:54

wolfgangwalther force-pushed the rollback-tests-default branch from 0294c31 to b7a6cf3 Compare November 24, 2020 23:14

steve-chavez reviewed Dec 3, 2020

View reviewed changes

test/Feature/SingularSpec.hs Show resolved Hide resolved

steve-chavez reviewed Dec 3, 2020

View reviewed changes

test/Feature/UpdateSpec.hs Show resolved Hide resolved

steve-chavez reviewed Dec 3, 2020

View reviewed changes

test/Feature/UpsertSpec.hs Show resolved Hide resolved

steve-chavez reviewed Dec 3, 2020

View reviewed changes

test/Feature/UpsertSpec.hs Show resolved Hide resolved

Change test-suite to use db-tx-rollback-all = true by default

ff75a76

fix tests when run repeatedly on same db

e01051e

wolfgangwalther force-pushed the rollback-tests-default branch from b7a6cf3 to e01051e Compare December 3, 2020 13:18

monacoremo mentioned this pull request Dec 3, 2020

[Meta] Developer experience improvements #1671

Closed

14 tasks

wolfgangwalther merged commit 609c9ae into PostgREST:master Dec 3, 2020

wolfgangwalther deleted the rollback-tests-default branch December 3, 2020 17:54

wolfgangwalther mentioned this pull request Apr 14, 2021

Add postgrest-loadtest based on vegeta #1812

Merged

6 tasks

monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021

Make transaction-rollback=true the default for test-suite (PostgREST#…

e60a285

…1663) Change test-suite to use db-tx-rollback-all = true by default

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make transaction-rollback=true the default for test-suite #1663

Make transaction-rollback=true the default for test-suite #1663

wolfgangwalther commented Nov 21, 2020

wolfgangwalther commented Nov 21, 2020 •

edited

Loading

steve-chavez commented Nov 24, 2020

wolfgangwalther commented Nov 24, 2020

steve-chavez commented Dec 3, 2020 •

edited

Loading

wolfgangwalther commented Dec 3, 2020

wolfgangwalther commented Dec 3, 2020

wolfgangwalther commented Dec 3, 2020

steve-chavez commented Dec 3, 2020

Make transaction-rollback=true the default for test-suite #1663

Make transaction-rollback=true the default for test-suite #1663

Conversation

wolfgangwalther commented Nov 21, 2020

wolfgangwalther commented Nov 21, 2020 • edited Loading

steve-chavez commented Nov 24, 2020

wolfgangwalther commented Nov 24, 2020

steve-chavez commented Dec 3, 2020 • edited Loading

wolfgangwalther commented Dec 3, 2020

wolfgangwalther commented Dec 3, 2020

wolfgangwalther commented Dec 3, 2020

steve-chavez commented Dec 3, 2020

wolfgangwalther commented Nov 21, 2020 •

edited

Loading

steve-chavez commented Dec 3, 2020 •

edited

Loading