Increasing test coverage for errors thrown when parsing entity definitions

[danbroooks]

Ups the test coverage in QuasiSpec to cover cases when there is an error, and make an assertion on the error message.

The original motivation for this was discovering that this error here:

persistent/persistent/Database/Persist/Quasi/Internal.hs

Lines 1155 to 1157 in aeaa4f6

	getDBName [] t =
	error $ "Unknown column in unique constraint: " ++ show t
	++ " " ++ show defs ++ show n ++ " " ++ show attrs

Is actually showing all the UnboundFields as illustrated in this commit prior to fixing:

89b34a9#diff-31a6cff4e15cd1978c97554cd4fae33101b142ab0e6a6e03f427458d2851b26bR457

This PR corrects this, and extracts a function to generate the error message in a more type safe way.

Further to this I have added some similar tests around other errors in this module which do not appear to have coverage.

---

Before submitting your PR, check that you've:

• Documented new APIs with Haddock markup
• Added @since declarations to the Haddock
• Ran stylish-haskell on any changed files.
• Adhered to the code style (see the .editorconfig file for details)

After submitting your PR:

• Update the Changelog.md file with a link to your PR
• Bumped the version number if there isn't an (unreleased) on the Changelog
• Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

[danbroooks]

Initially I implemented this test the same was as the unterminated parens test:

        it "should error on malformed input, unterminated parens" $ do
            let definitions = [st|
User
    name Text
    age  (Maybe Int
|]
            let [user] = parse lowerCaseSettings definitions
            evaluate (unboundEntityDef user)
                `shouldThrow`
                    errorCall "Unterminated parens string starting with Maybe Int"

However I wasn't able to come up with a realistic looking example, the following entity def did not appear to trigger this error:

        it "should error on malformed input, unterminated quotes" $ do
            let definitions = [st|
User
    name Text
    created UTCTime default="NOW()
|]
            let [user] = parse lowerCaseSettings definitions
            evaluate (unboundEntityDef user)
                `shouldThrow`
                    errorCall "error text goes here"

However this did not seem to trip up the parser. It seems like the parser is expecting a token that begins with a quote, as illustrated here. What is this required for in persistent? I couldnt think of anything besides the above default example and was unable to find examples of this in the repo.

Furthermore, I deleted this line from the tokenizer and the tests carried on working fine:

persistent/persistent/Database/Persist/Quasi/Internal.hs

Line 207 in aeaa4f6

| T.head t == '"' = quotes (T.tail t) id

So I am wondering if this something that was added that could possibly be made redundant now?

I am not removing that line in this PR though as I feel like it's asking for trouble 😄

[parsonsmatt]

Hmmm, intriguing. The whole parser stuff is kind of a mess. I'd love to define a megaparsec parser (or, for funsies, an Alex/Happy situation). I'd also be hesitant to remove the code unless I could prove it was unreachable, or if we had some test cases that could reach it

[danbroooks]

So... 😄 this is actually what I have been tinkering around with this weekend, with some success:

master...danbroooks:parser-refactor-spike

☝️ the above seems to work against the current set of unit tests, I was going to open an issue to see if there was an appetite for a change like this (as obviously its a fairly big shift in style for the quasi parser currently), I don't see why not if it doesn't bring in any extra dependencies, I guess... it's just if test coverage here is sufficient enough for a change like this (though it seems so to me?)

Also, you may have your own vision for the direction you want to take the parser in, I'd be happy to work on some changes like that steered in a particular direction, the above branch is just my take on it after a weekend hacking away at it 😄

In terms of this comment + PR, I guess these tests at least model this current behaviour, even if it is potentially unreachable/unused, I can leave this code alone for the time being 😄

[parsonsmatt]

Yeah! That's awesome. I'd be happy to have that in place.

Switching to a more reasonable parser combinator style would be awesome, even if it is home-baked. I'm fine bringing in megaparsec, too.