Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add property-based tests (some are failing) #12

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

seriyps
Copy link

@seriyps seriyps commented Jan 26, 2022

Mostly based the generators on the typespecs in thoas module. Some of it borrowed from my old gist https://gist.github.com/seriyps/ffe0f5f5ba270fc4dd395208718e84d3

It fails on some input, CI would fail because of that:

  • escape => html sometimes fails
  • [{K, V}] representation of JSON objects seems to not be supported

Also I noticed typespec misses null

* `escape => html` sometimes fails
* `[{K, V}]` representation of JSON objects seems to not be supported
Copy link
Owner

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, and very cool to see how a bug has been found. I've left some minor notes inline and then we can merge or alternatively fix the bug then merge, depending on your preference.

test/prop_thoas.erl Outdated Show resolved Hide resolved
Comment on lines 3 to 5
-export([
json_term/0, input_term/0
]).
Copy link
Owner

Choose a reason for hiding this comment

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

Do these need to be exported? I could not see where they were used otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

Not really, but they were quite useful during development:

$ rebar3 as test shell
> proper_gen:pick(prop_thoas:json_term()).
{ok,#{<<>> => true,<<"I">> => 9.51697150738206,<<"TkxRF">> => 18,
      <<"Ylptz">> =>
          [false,
           <<230,167,155,200,141,238,134,152,56,7,230,191,141>>,
           false,-4.303609167549358,true,null,null],
      <<"p5kzkXK3pQ">> =>
          [<<238,158,129,54,204,185,195,129>>,
           null,null,false,false,-2.377673467827075,<<"yPCxZPihb">>,
           true]}}

test/prop_thoas.erl Outdated Show resolved Hide resolved
test/prop_thoas.erl Outdated Show resolved Hide resolved
test/prop_thoas.erl Outdated Show resolved Hide resolved
test/prop_thoas.erl Outdated Show resolved Hide resolved
test/prop_thoas.erl Outdated Show resolved Hide resolved
test/prop_thoas.erl Outdated Show resolved Hide resolved
test/prop_thoas.erl Outdated Show resolved Hide resolved
test/prop_thoas.erl Outdated Show resolved Hide resolved
@seriyps
Copy link
Author

seriyps commented Jan 26, 2022

Thanks for the feedback. Waiting for your answer about typespec (and null as well) and formatter.

we can merge or alternatively fix the bug then merge, depending on your preference.

I don't have any strong preference. If you are fine with red CI on master, guess might make sense to merge this PR first since it is already here. But quite unlikely that I'm going to work on the bug fix unfortunately.

@lpil
Copy link
Owner

lpil commented Jan 26, 2022

But quite unlikely that I'm going to work on the bug fix unfortunately.

Sounds like we want it merged then. The main branch doesn't deploy anywhere so a broken CI job is OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants