Servant Swagger QuickCheck Property Testing (validateEveryJSON & validateEveryPath) #108

KtorZ · 2019-03-21T19:34:12Z

Issue Number

Overview

I have added a serie of property based tests to verify that every type used with JSON content type in a servant API has compatible ToJSON and ToSchema instances.
I have added a serie of property based tests to verify that every path specified by the servant server matches an existing path in the specification

Comments

Upon failure, the former gives us a nice output with the resolved swagger schema for the corresponding resource and the counter example in JSON (after shrinking):

see 'validateEveryToJSON' test case failure

1) Cardano.Wallet.Api.Types, api matches the swagger specification, Wallet
     Falsifiable (after 1 test and 11 shrinks):
       Wallet {_id = ApiT {getApiT = WalletId 00000000-0000-0000-0000-000100000001}, _addressPoolGap = ApiT {getApiT = AddressPoolGap {getAddressPoolGap = 32}}, _balance = ApiT {getApiT = WalletBalance {_available = MeasuredIn 0, _total = MeasuredIn 0}}, _delegation = ApiT {getApiT = NotDelegating}, _name = ApiT {getApiT = WalletName {getWalletName = "a"}}, _passphrase = ApiT {getApiT = WalletPassphraseInfo {lastUpdatedAt = 1858-11-17 00:00:00 UTC}}, _state = ApiT {getApiT = Restoring (MeasuredIn 38)}}
       Validation against the schema fails:
         * property "patate" is required, but not found in "{\"passphrase\":{\"last_updated_at\":\"1858-11-17T00:00:00Z\"},\"address_pool_gap\":32,\"state\":{\"status\":\"restoring\",\"progress\":{\"quantity\":38,\"unit\":\"percent\"}},\"balance\":{\"total\":{\"quantity\":0,\"unit\":\"lovelace\"},\"available\":{\"quantity\":0,\"unit\":\"lovelace\"}},\"name\":\"a\",\"delegation\":{\"status\":\"not_delegating\"},\"id\":\"00000000-0000-0000-0000-000100000001\"}"
       
       JSON value:
       {
           "passphrase": {
               "last_updated_at": "1858-11-17T00:00:00Z"
           },
           "address_pool_gap": 32,
           "state": {
               "status": "restoring",
               "progress": {
                   "quantity": 38,
                   "unit": "percent"
               }
           },
           "balance": {
               "total": {
                   "quantity": 0,
                   "unit": "lovelace"
               },
               "available": {
                   "quantity": 0,
                   "unit": "lovelace"
               }
           },
           "name": "a",
           "delegation": {
               "status": "not_delegating"
           },
           "id": "00000000-0000-0000-0000-000100000001"
       }
       
       Swagger Schema:
       {
           "required": [
               "id",
               "address_pool_gap",
               "balance",
               "delegation",
               "name",
               "passphrase",
               "state",
               "patate"
           ],
           "type": "object",
           "properties": {
               "passphrase": {
                   "required": [
                       "last_updated_at"
                   ],
                   "type": "object",
                   "description": "Information about the wallet's passphrase",
                   "properties": {
                       "last_updated_at": {
                           "example": "2019-02-27T14:46:45Z",
                           "format": "iso-8601-date-and-time",
                           "type": "string"
                       }
                   }
               },
               "address_pool_gap": {
                   "example": 20,
                   "maximum": 100,
                   "default": 20,
                   "minimum": 10,
                   "type": "integer",
                   "description": "Number of consecutive unused addresses allowed"
               },
               "state": {
                   "example": {
                       "status": "ready"
                   },
                   "required": [
                       "status"
                   ],
                   "type": "object",
                   "description": "Whether a wallet is ready to use or still syncing",
                   "properties": {
                       "status": {
                           "type": "string",
                           "enum": [
                               "ready",
                               "restoring"
                           ]
                       },
                       "progress": {
                           "required": [
                               "quantity",
                               "unit"
                           ],
                           "type": "object",
                           "description": "Only present if status `restoring`",
                           "properties": {
                               "quantity": {
                                   "example": 42,
                                   "maximum": 100,
                                   "minimum": 0,
                                   "type": "number"
                               },
                               "unit": {
                                   "type": "string",
                                   "enum": [
                                       "percent"
                                   ]
                               }
                           }
                       }
                   }
               },
               "balance": {
                   "required": [
                       "available",
                       "total"
                   ],
                   "type": "object",
                   "description": "Wallet current balance(s)",
                   "properties": {
                       "total": {
                           "required": [
                               "quantity",
                               "unit"
                           ],
                           "type": "object",
                           "description": "Total balance (available balance plus pending change)",
                           "properties": {
                               "quantity": {
                                   "example": 42000000,
                                   "minimum": 0,
                                   "type": "integer"
                               },
                               "unit": {
                                   "type": "string",
                                   "enum": [
                                       "lovelace"
                                   ]
                               }
                           }
                       },
                       "available": {
                           "required": [
                               "quantity",
                               "unit"
                           ],
                           "type": "object",
                           "description": "Available balance (funds that can be spent)",
                           "properties": {
                               "quantity": {
                                   "example": 42000000,
                                   "minimum": 0,
                                   "type": "integer"
                               },
                               "unit": {
                                   "type": "string",
                                   "enum": [
                                       "lovelace"
                                   ]
                               }
                           }
                       }
                   }
               },
               "name": {
                   "maxLength": 255,
                   "example": "Alan's Wallet",
                   "minLength": 1,
                   "type": "string"
               },
               "delegation": {
                   "example": {
                       "status": "delegating",
                       "target": "2cWKMJemoBam7gg1y5K2aFDhAm5L8fVc96NfxgcGhdLMFTsToNAU9t5HVdBBQKy4iDswL"
                   },
                   "required": [
                       "status"
                   ],
                   "type": "object",
                   "description": "Delegation settings",
                   "properties": {
                       "status": {
                           "type": "string",
                           "enum": [
                               "not_delegating",
                               "delegating"
                           ]
                       },
                       "target": {
                           "example": "2cWKMJemoBam7gg1y5K2aFDhAm5L8fVc96NfxgcGhdLMFTsToNAU9t5HVdBBQKy4iDswL",
                           "format": "base58",
                           "type": "string",
                           "description": "A unique Stake-Pool identifier (present only if status = `delegating`)"
                       }
                   }
               },
               "patate": {
                   "type": "string",
                   "enum": [
                       "autruche",
                       "patate"
                   ]
               },
               "id": {
                   "example": "a312e413-e8c8-4bda-9f5b-9b7f518174d8",
                   "format": "uuid",
                   "type": "string",
                   "description": "A unique identifier for the wallet"
               }
           }
       }

The latter constructs an spec tree with one test case per API endpoint. In the form of:

  verify that every path specified by the servant server matches an existing path in the specification
    DELETE /patate/{walletId} exists in specification FAILED [1]
    GET /wallets/{walletId} exists in specification
    GET /wallets exists in specification

If one of them is failing, it is reported as:

  test/unit/Cardano/Wallet/Api/TypesSpec.hs:249:9: 
  1) Cardano.Wallet.Api.Types, verify that every path specified by the servant server matches an existing path in the specification, DELETE /patate/{walletId} exists in specification
       uncaught exception: IOException of type UserError
       user error (couldn't find path in specification)

If the specification document is missing the code won't even compile and fail with a rather descriptive message.
If the specification document can't be parsed correctly, because the yaml is invalid, the tests will fail.

KtorZ · 2019-03-21T19:34:47Z

⚠️ Beware, the base branch targets #107 ⚠️

jonathanknowles

I think this PR strikes a nice balance between what is possible and what is practical. We get the benefit of verifying some important properties, without writing a complete Swagger->Servant generator.

I guess at some point we may want to add another test, namely that:

"every path that exists in the Swagger specification also exists as a path in our Servant API."

However, I think it's obvious that such a test will fail unless we really do have a complete API. Therefore, we can probably wait until we do have a complete API before adding such a test.

There are some minor points in my review that hopefully could be addressed, but otherwise I think this is good. 👍

jonathanknowles · 2019-03-22T04:59:35Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

-        ("can perform roundtrip JSON serialization & deserialization, " <>
-         "and match existing golden files") $ do
+        "can perform roundtrip JSON serialization & deserialization, \
+        \and match existing golden files" $ do


jonathanknowles · 2019-03-22T05:01:33Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+         - Parsing the specification file (which is embedded at compile-time)
+         - Creating missing 'ToSchema' by doing lookups in that global schema
+
+  2/ The above verification is rather week, because it just controls the return


The above verification is rather week

Small typo, should be:

The above verification is rather weak

jonathanknowles · 2019-03-22T05:03:18Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+        Just schema ->
+            return $ NamedSchema (Just "Wallet") schema
+
+-- | Verify that every servant endpoints are present and match the specification


Verify that every servant endpoints are present and match the specification

Minor point, should be:

Verify that all servant endpoints are present and match the specification

test/unit/Cardano/Wallet/Api/TypesSpec.hs

jonathanknowles · 2019-03-22T07:30:10Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+     The trick is for the later point. In a "classic" scenario, we would have
+     defined the `ToSchema` instances directly in Haskell on our types, which
+     eventually becomes a real pain to maintain. Instead, we have written the
+     spec by hand, and we want to control that our implementation matches it.


and we want to control that our implementation matches it.

Recommend changing the wording to:

and we want to check that our implementation matches it.

jonathanknowles · 2019-03-22T07:31:13Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+
+  2/ The above verification is rather week, because it just controls the return
+     types of endpoints, but not that those endpoints are somewhat valid. Thus,
+     we've also built another check 'validateEveryPath' which crawl our servant


crawl

should be

crawls

jonathanknowles · 2019-03-22T07:31:45Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+  2/ The above verification is rather week, because it just controls the return
+     types of endpoints, but not that those endpoints are somewhat valid. Thus,
+     we've also built another check 'validateEveryPath' which crawl our servant
+     API type, and check whether every path we have in our API appears in the


check

should be

checks

test/unit/Cardano/Wallet/Api/TypesSpec.hs

jonathanknowles · 2019-03-22T08:11:04Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

+         - construct the corresponding path (with verb)
+         - build an HSpec scenario which checks whether the path is present
+    This seemingly means that the identifiers we use in our servant paths (in
+    particular, those for path parameters) should exactly match the specs.


I guess at some point we may want to add another test, namely that:

"every path that exists in the Swagger specification also exists as a path in our Servant API."

But such a test will fail unless we really do have a complete API, so we can only add it when we do have a complete API.

Yes. Thought of it indeed but in practice, we will likely modify and extend the specification always before adding the corresponding implementation. We could also just have "stubs" in the Haskell with empty handlers throwing 501. It could work quite fine without much overhead when modifying specs.

KtorZ · 2019-03-22T08:31:07Z

@jonathanknowles Wording & Grammar (& line formatting) reviewed 👍

Addressed gramatical & styling points in: https://github.com/input-output-hk/cardano-wallet/compare/2446e6779f87586a2fa016b50122e90551ac1fc6..904c023325f0f0f9bd8bb379b198a2face7e2741

akegalj

💯

akegalj · 2019-03-22T10:22:14Z

src/Cardano/Wallet/Api/Types.hs

@@ -67,13 +67,13 @@ import qualified Data.Aeson.Types as Aeson
 -------------------------------------------------------------------------------}

 data Wallet = Wallet
-    { _id :: !WalletId
+    { _id :: !(ApiT WalletId)


is there a reason we are wrapping all in ApiT that I am not aware off?

Or we explicitly want to keel all json instances defined in this module?

aha - we probably want to enforce the same genericToJSON and genericParseJSON rules for the whole api, so we are going to wrap most/all things with ApiT?

-> Or we explicitly want to keel all json instances defined in this module?

You said it :)
A nice explanation from @jonathanknowles here: #107 (comment)

KtorZ · 2019-03-22T11:36:50Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

            roundtripAndGolden $ Proxy @ Wallet
            roundtripAndGolden $ Proxy @ (ApiT AddressPoolGap)
-            roundtripAndGolden $ Proxy @ (ApiT WalletBalance)


Duplicate with a line below

…s we generate are actually compliant with the spec

…ance with the API specification

Anviking

🎉Awesome!

Observation: noting that we're (obviously) not checking that everything in the api-spec exists in our API-type — ah which you essentially also mentioned

Anviking · 2019-03-22T11:41:21Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

            roundtripAndGolden $ Proxy @ (ApiT (WalletDelegation (ApiT PoolId)))
            roundtripAndGolden $ Proxy @ (ApiT WalletId)
            roundtripAndGolden $ Proxy @ (ApiT WalletName)
            roundtripAndGolden $ Proxy @ (ApiT WalletBalance)
            roundtripAndGolden $ Proxy @ (ApiT WalletPassphraseInfo)
            roundtripAndGolden $ Proxy @ (ApiT WalletState)

-- | Run JSON roundtrip & golden tests


Why remove this line?

🤔 Good question? Probably merge-conflict resolution that went little wrong here.

Anviking · 2019-03-22T14:01:35Z

specifications/api/swagger.yaml

 #                                                                           #
 #############################################################################

-address: &address


I am assuming you have just relocated and polished a few things here

Yes! I actually forgot about this while writing the spec file but, Swagger expects "definitions" of model / resources to be located in a "definitions" properties from the root swagger file. So I just moved things to be located under this field.

KtorZ requested review from jonathanknowles and Anviking March 21, 2019 19:34

KtorZ changed the base branch from master to KtorZ/53/finalize-first-api-types March 21, 2019 19:34

jonathanknowles mentioned this pull request Mar 21, 2019

Translate the Swagger API specification into a Servant API specification. #53

Closed

3 tasks

jonathanknowles previously requested changes Mar 22, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/53/servant-swagger-quickcheck branch from 2446e67 to 904c023 Compare March 22, 2019 08:30

KtorZ self-assigned this Mar 22, 2019

KtorZ force-pushed the KtorZ/53/servant-swagger-quickcheck branch from 904c023 to 7fcba48 Compare March 22, 2019 09:26

KtorZ force-pushed the KtorZ/53/finalize-first-api-types branch 2 times, most recently from 226a37b to df92987 Compare March 22, 2019 09:33

akegalj reviewed Mar 22, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/53/finalize-first-api-types branch from df92987 to d0ae386 Compare March 22, 2019 10:29

KtorZ changed the base branch from KtorZ/53/finalize-first-api-types to master March 22, 2019 10:57

KtorZ force-pushed the KtorZ/53/servant-swagger-quickcheck branch from 7fcba48 to 7a06db9 Compare March 22, 2019 11:36

KtorZ commented Mar 22, 2019

View reviewed changes

KtorZ added 2 commits March 22, 2019 12:37

add 'validateEveryJSON' from servant-swagger to control that API type…

4723505

…s we generate are actually compliant with the spec

implement a 'validateEveryPath' to add extra guarantees on our compli…

25d5c9e

…ance with the API specification

Anviking approved these changes Mar 22, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/53/servant-swagger-quickcheck branch from 7a06db9 to 25d5c9e Compare March 22, 2019 14:45

KtorZ merged commit 1d984dc into master Mar 22, 2019

KtorZ deleted the KtorZ/53/servant-swagger-quickcheck branch March 22, 2019 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Servant Swagger QuickCheck Property Testing (validateEveryJSON & validateEveryPath) #108

Servant Swagger QuickCheck Property Testing (validateEveryJSON & validateEveryPath) #108

KtorZ commented Mar 21, 2019 •

edited

Loading

KtorZ commented Mar 21, 2019

jonathanknowles left a comment

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

KtorZ Mar 22, 2019

KtorZ commented Mar 22, 2019 •

edited

Loading

akegalj left a comment

akegalj Mar 22, 2019

KtorZ Mar 22, 2019

KtorZ Mar 22, 2019

Anviking left a comment •

edited

Loading

Anviking Mar 22, 2019

KtorZ Mar 22, 2019

Anviking Mar 22, 2019

KtorZ Mar 22, 2019

Servant Swagger QuickCheck Property Testing (validateEveryJSON & validateEveryPath) #108

Servant Swagger QuickCheck Property Testing (validateEveryJSON & validateEveryPath) #108

Conversation

KtorZ commented Mar 21, 2019 • edited Loading

Issue Number

Overview

Comments

KtorZ commented Mar 21, 2019

jonathanknowles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Mar 22, 2019 • edited Loading

akegalj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Mar 21, 2019 •

edited

Loading

KtorZ commented Mar 22, 2019 •

edited

Loading

Anviking left a comment •

edited

Loading