The first step towards validity-based testing.

[NorfairKing]

This path library is very scarsely tested. I would like to improve that.
There are invariants for the Path type.
We should:

1. Make those invariants explicit
2. Make sure that all the functions that produce a Path, can only produce valid Paths.
3. Add a bunch of property tests.
4. Add doctests to show what functions should (intuitively) do
5. Add/Generate a bunch of unit tests to ensure that the API is invariant under refactors/optimisations
   (6.) Optimise the internals to use something better than String.

This commit just takes the first step by adding a Validity instance for Paths.

I would like to hear your thoughts on the matter.
See https://github.com/NorfairKing/safepath for the level of testing I would like to achieve here.

[NorfairKing]

Just saw your continuous builds.
I don't think it will be possible to support the tests I describe as far back as 7.0 .

[mrkkrp]

I'm having hard time seeing any value in this PR. Depending on your validity does not improve the package, is it already checks all the things (if not all, add new checks to the smart constructors at least) and does it in a more flexible way with MonadThrow. BTW, validity already exists in more advanced form as refined.

[mrkkrp]

Why would you want to do (6)? All the useful functions take FilePath anyway, which is a String, so if you use something else, you'll need to convert back and forth actually degrading performance, not improving it. In its present state path does the best thing possible wrapping FilePath without run-time overhead.

[NorfairKing]

This PR is not done, I would go at least through the first 5 steps (6 was optional anyway) before merging anything. I use validity because then I can do testing with genvalidity-hspec.
I stopped here for now to see how open you would be to this kind of contribution.

[NorfairKing]

See https://github.com/NorfairKing/safepath/blob/master/safepath-testing/test/Data/PathSpec.hs for the kind of tests I mean.

[phadej]

As I said elsewhere, depending on validity (or refined) is something a maintainer should decide on, as one doesn't necessarily need an external package to define which value is valid.

E.g.

-- | Convert a relative 'FilePath' to a normalized relative dir 'Path'.
--
-- Throws: 'PathParseException' when the supplied path:
--
-- * is not a relative path
-- * is any of @""@, @.@ or @..@
-- * contains @..@ anywhere in the path
-- * is not a valid path (See 'System.FilePath.isValid')
--
parseRelDir :: MonadThrow m
            => FilePath -> m (Path Rel Dir)

IMHO it makes sense to encode Throws clauses as properties checked with doctest. Currently test-suite contains only examples AFAICS.

I'm 👍 to the goals of this PR.

[chrisdone]

The validity library looks pretty neat now that I see your PR with it. Presumably that instance lets you mess around with generated values while ensuring that the values are correct.

I'm a little concerned that you might be underwhelmed with how many tests are needed for the path once you start. On the other hand if that's the case you won't have much work to do. 👍

I'm not too concerned about making the test-suite require > ghc-7.0, if needs be.

[NorfairKing]

The validity dependency can be moved to the tests actually. It will be an orphan instance then, but that would solve the dependecy problem.

[NorfairKing]

The validity library looks pretty neat now that I see your PR with it. Presumably that instance lets you mess around with generated values while ensuring that the values are correct.

You are mixing up Validity and GenValidity but I think you understand the concept.
https://hackage.haskell.org/package/validity vs https://hackage.haskell.org/package/genvalidity

I'm not too concerned about making the test-suite require > ghc-7.0, if needs be.

Great!

[NorfairKing]

I added the first few validity tests.
If this looks like something you would want to merge, let me know and I will go on testing some more.

[NorfairKing]

I may have screwed up the commit message there: I found a false positive because the validity instance is not strict enough yet. Stil WIP, but would like to hear your thoughts. You may want to view the three commits together instead of seperately.

[chrisdone]

The commit looks pretty simple and uninvasive. I like it so far.

One question, how does this differ to implementing QuickCheck property-based tests?

[NorfairKing]

One question, how does this differ to implementing QuickCheck property-based tests?

I'm not sure I understand the question.
I am using property-based tests.

[chrisdone]

Oh, of course. I'm merging into master and I'll try to contribute some more properties too.

For example the "parent" function is in need of a breaking change to its type (tracked in another issue) which someone pointed out contradicted a properties purported by the docs, it would be cool to make that triggered in these property tests, and then fix the bug. I'll get to that tomorrow.

[chrisdone]

Tomorrow I'd like to make sure that the build matrix is all green, when I'm back at my desk. Seems like it's not at the moment.

Thanks Tom!

[NorfairKing]

@chrisdone Thank you very much for the chance to contribute.
I'd like to contribute some more, if that's okay. (In another PR).