Skip to content

[#32] Add InsertLookup property test for PrefixTree#34

Merged
chshersh merged 1 commit intomasterfrom
chshersh/32-prefix-property
May 1, 2018
Merged

[#32] Add InsertLookup property test for PrefixTree#34
chshersh merged 1 commit intomasterfrom
chshersh/32-prefix-property

Conversation

@chshersh
Copy link
Copy Markdown
Contributor

@chshersh chshersh commented May 1, 2018

Resolves #32

Implements InsertLookup property test for PrefixTree. I can't think of more properties for now... Maybe more of them can be discovered in future.

I've also added some unit tests.

@chshersh chshersh added test Testing (unit, properties) PrefixTree PrefixTree and PrefixMap related stuff labels May 1, 2018
@chshersh chshersh added this to the v0.0.0: Initial release milestone May 1, 2018
@chshersh chshersh self-assigned this May 1, 2018
@chshersh chshersh requested a review from vrom911 May 1, 2018 00:54
Copy link
Copy Markdown
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like what is going on there! Cool one, thank you! I just wrote few comments in there 🙂

Comment thread src/Toml/PrefixTree.hs Outdated
deriving (Show, Eq, Hashable)

instance IsString Piece where
fromString = Piece . fromString
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made the same thing 🙂 I just derived :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I should derive it as well...

@@ -0,0 +1,13 @@
module Test.Toml.PrefixTree
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this structure! 👍 It makes code easier to read with this test separation

pure $ HashMap.fromList kvps

genPrefixTree :: forall m . MonadGen m => Key -> m (PrefixTree V)
genPrefixTree key = Gen.recursive
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nice to see this library feature to work for us! 👏


-- DEBUG: ensures that trees of depth at least 5 are generated
-- assert $ depth prefMap < 5

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another property I can think of is

lookup a (insert a y $ insert a x t) === Just y

to check that it uses the latest value. How do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrom911 That's an excellent property! I can add it as well.

@chshersh chshersh force-pushed the chshersh/32-prefix-property branch from 6a27cf5 to 599131d Compare May 1, 2018 15:28
Copy link
Copy Markdown
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍬

@chshersh chshersh merged commit d27c4d3 into master May 1, 2018
@chshersh chshersh deleted the chshersh/32-prefix-property branch May 1, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PrefixTree PrefixTree and PrefixMap related stuff test Testing (unit, properties)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Property tests for 'PrefixTree' and 'PrefixMap'

2 participants