Property Based Tests by clark-lindsay · Pull Request #9 · mhanberg/schematic

clark-lindsay · 2023-04-28T23:00:31Z

Description

Adds property-based tests with StreamData to improve confidence in the test suite and make the library more reliable and easier to extend.

Most of the property based testing is condensed around a single property test that asserts that an arbitrary {schematic, data} tuple, where the data and the schematic are compatible and will successfully unify/2, satisfies the following pseudocode: data |> unify |> dump == data

As this is a draft, i am open to any and all feedback

Closes #8

mhanberg

I'll have to pull this down and play around with it, nice work so far!

mhanberg · 2023-04-30T17:06:26Z

Does it make sense to organize the prop tests based on "properties" of the code rather than by function?

like, the property is that inputs can be unified and dumped back and forth, so the stream data generator generates random arbitrary inputs/schematics and ensures that input == unify(schematic, input) |> then(&dump(schematic, &1))

I assume it's currently broken up by function because you were just incrementally working through them, but before we merge it is worth considering whether combining them into fewer properties makes sense.

mhanberg · 2023-05-01T03:10:56Z

+    |> StreamData.one_of()
+  end
+
+  def from_schematic(%Schematic{kind: kind}) do


I think this is closer to what we want.

I think there should probably be some function like gen or generate that takes a schematic, and then spits out the stream data equivalent.

I think you still need to move the stream data generator for the blueprintable map schematic into this.

Then, I feel like you would be able to to more or less have a single property that generates an arbitrary schematic that can be any depth and combination of them, and test that it unifies and dumps back to itself.

And this functionality can probably be re-used to make the user facing stream data API

i just added something that is getting towards this idea. it's a little rough, and doesn't cover key transformation for maps, but with this is place we can probably drop a lot of the smaller property tests that i wrote along the way to this. i'll take a look at that tomorrow

mhanberg · 2023-05-01T03:13:39Z

+      check all(
+              {blueprint, input} <-
+                StreamData.bind(
+                  StreamData.map_of(StreamData.binary(), Generators.schematic(), min_length: 1),


map schematics can technically have any value as a key in the blueprint, so we would want to expand this generator to reflect that.

We'll also want it to generate map blue prints that reflect the key transformation capabilities: https://hexdocs.pm/schematic/Schematic.html#map/1-transforming-keys

clark-lindsay · 2023-05-01T19:52:55Z

Does it make sense to organize the prop tests based on "properties" of the code rather than by function?

like, the property is that inputs can be unified and dumped back and forth, so the stream data generator generates random arbitrary inputs/schematics and ensures that input == unify(schematic, input) |> then(&dump(schematic, &1))

I assume it's currently broken up by function because you were just incrementally working through them, but before we merge it is worth considering whether combining them into fewer properties makes sense.

yeah, i think that that makes sense. i didn't jump into this with a wholistic understanding of everything, so yeah i was just working through incrementally. now that i have a better understanding of the shape of the whole thing, i think this makes sense

mhanberg · 2023-05-02T15:31:30Z

+    property "input |> unify |> dump == input" do
+      check all({schematic, input} <- Generators.schematic_and_data()) do
+        {:ok, input} ==
+          unify(schematic, input) |> then(fn {:ok, result} -> dump(schematic, result) end)
+      end
+    end


will this replace the other properties?

most of them, i think. i'm gonna comb through today and remove the ones that are definitely overshadowed by this

replaced a bunch of existing properties that were superseded by this one. still a few remaining, but i suspect only one or two besides this are worth keeping. lmk if you feel differently

mhanberg · 2023-05-03T01:10:16Z

One annoying comment, can you add stream_data to the formatter.exs import deps and then remove the parens from all the all instances?

😅

clark-lindsay · 2023-05-03T15:19:56Z

One annoying comment, can you add stream_data to the formatter.exs import deps and then remove the parens from all the all instances?

😅

haha sure thing. not "annoying" btw, i totally get it

mhanberg · 2023-05-04T00:54:21Z

@clark-lindsay Can you rebase this and then I'll review

WORK IN PROGRESS: may end up removing the tests that the new property-based tests overshadow, which would do away with the awkward naming convention that i started with. additionally, i wrote the generators in the `test/support` directory after the JSON spec because it seemed to match well with the data types supported by `schematic` right now (w/ the exception of `tuple`), but this may just be incidental

WORK IN PROGRESS: there are a few rough edges here, so i'll record my current thoughts on them: - i'm still not sure how valuable a separate module for `StreamData` generators is, but right now it feels like it improves the organization of the test code - currently a `list/0` schematic can match a non-homogenous list, whereas any call to `list/1` must match the given schematic homogenously (but you **could** pass a `one_of` schematic...) - is this the right design? i think it ***is right***, but i left myself a "TODO" to think about it some more - should property-based tests be written to describe the properties of the code for the failure paths? or is that better left to hand-written edge case unit tests?

also extended some of the data generation in the `Generators` test support module

- better reporting from `mix test` - easier to notice/find/grep-for tests are property-based - automatically tags the tests, so one can use `mix test --only property` to target _just_ the property-based tests

also extend and start to document support generators, in support of ongoing test expansion and improvements

i don't entirely understand why there was a call to `Map.delete/2` in the `unify` definition for the `map` schematic, but with it in place i was dropping `""` keys when `dump`-ing. removing the call has no effect on any other tests, and so i believe that it's not necessary, ***but*** i may have written the test incorrectly and misunderstood the desired behaviour...

i still can't think of a good way to generate tuples, but i at least want the schematic `kind` accounted for in the tests in the meantime

@motch

thanks @motch for the _much_ improved name idea

the `property` "input |> unify |> dump == input" supercedes most of the existing `property`s in the test file, which were written as stepping stones for me to understand the behaviours and design of the code to work towards more general properties that could be tested remaining `property`s that were not deleted still have cases that are not covered by the aforementioned "input |> unify |> dump == input" test, and some `TODO`s have been added to address the generation of schematics and data so that these `property`s will become redundant and then be removed

also: - extended the schematics that can be generated from `Generators.schematic_and_data/0` - removed a `property` that is now superseded by the "input |> unify |> dump |> input" `property` test - added a few specific cases to the `tuple/2` example-based tests that reproduced the bug found in the `unify` callback from `tuple/2`

Co-authored-by: Mitchell Hanberg <mitch@mitchellhanberg.com>

mhanberg · 2023-05-04T20:10:27Z


  ```elixir
-  iex> schematic = tuple([str(), int()])
+  iex> schematic = Schematic.tuple([str(), int()])


did you say you could remove the Schematic. part?

mhanberg · 2023-05-05T01:11:29Z

Thanks!

mhanberg reviewed Apr 29, 2023

View reviewed changes

Comment thread test/schematic_test.exs Outdated

Comment thread test/schematic_test.exs Outdated

mhanberg reviewed May 1, 2023

View reviewed changes

Comment thread test/support/generators.ex Outdated

mhanberg reviewed May 1, 2023

View reviewed changes

mhanberg reviewed May 2, 2023

View reviewed changes

Comment thread test/support/generators.ex Outdated

clark-lindsay marked this pull request as ready for review May 3, 2023 22:05

mhanberg reviewed May 4, 2023

View reviewed changes

clark-lindsay added 17 commits May 4, 2023 11:34

chore: add property-based tests for oneof schematics

5b2802f

chore: add property-based tests for map unification

8544c75

also extended some of the data generation in the `Generators` test support module

chore: use property over test for better ergonomics

576c561

- better reporting from `mix test` - easier to notice/find/grep-for tests are property-based - automatically tags the tests, so one can use `mix test --only property` to target _just_ the property-based tests

chore: extend tuple/2 property test w/ from: :list option

445e7a4

chore: add more property tests for map/1

1058fdc

also extend and start to document support generators, in support of ongoing test expansion and improvements

chore: generate tuple schematics as empty {}

4b79bbd

i still can't think of a good way to generate tuples, but i at least want the schematic `kind` accounted for in the tests in the meantime

chore: add new generator and property-test for arbitrary schematics

219aba7

chore: remove unused generator

7dee024

chore: rename generator function for scalar data

335f48e

thanks @motch for the _much_ improved name idea

chore: import stream_data formatting & fix up formatting

2bfa92d

chore: improve data generation options and documentation

0d14864

chore: rename fns, reorganize, and add docs for Generators

2582227

clark-lindsay and others added 4 commits May 4, 2023 11:40

chore: update generator docs

bcd6400

Co-authored-by: Mitchell Hanberg <mitch@mitchellhanberg.com>

chore: fix up docs & use remote calls over imports to avoid confusion

e8da8a3

chore: clean up Generators implementation

1ec197c

fix: improve error messages and implementation of tuple/2

12d21ea

clark-lindsay force-pushed the add-property-based-tests branch from 1e02862 to 12d21ea Compare May 4, 2023 15:42

clark-lindsay added 2 commits May 4, 2023 13:40

fix: back-fill then/2 for older elixir versions

8bbc2f1

fix: ensure :stream_data is started for elixir versions < 1.11

8963382

mhanberg reviewed May 4, 2023

View reviewed changes

chore: update doc

964d53c

mhanberg merged commit 44ac388 into mhanberg:main May 5, 2023

Conversation

clark-lindsay commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

mhanberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhanberg commented Apr 30, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clark-lindsay commented May 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhanberg commented May 3, 2023

Uh oh!

clark-lindsay commented May 3, 2023

Uh oh!

mhanberg commented May 4, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhanberg commented May 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clark-lindsay commented Apr 28, 2023 •

edited

Loading