-
Notifications
You must be signed in to change notification settings - Fork 168
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
Parallel crash fix, #224 #227
base: master
Are you sure you want to change the base?
Conversation
d6edbc6
to
6c67c7d
Compare
@kostis it's no longer WIP and is ready for review. |
6c67c7d
to
8b66ae0
Compare
Thanks for the #224 issue and this PR. If all goes well, I will look into this before the end of the week. In the meantime, you may want to correct an erroneous |
6a63a89
to
7338f60
Compare
Typespec of `proper_statem:execute/3` had to be changed to return an ok|error tuple so the older test had to be adjusted as well.
7338f60
to
9fce322
Compare
OK, it took longer than expected because in the meantime I decided to first increase the test suite coverage and then to fix two statem tests to actually test what they are supposed to, but now I have looked at this. Long story short: I do not (fully) understand the issue and I have doubts that the test shows what you think it shows. I would like to see a test where a property like this one prop_exception() ->
?FORALL(Cmds, commands(?MODULE),
begin
{_History,_State,Result} = run_commands(?MODULE, Cmds),
Result =:= ok
end). fails because prop_parallel_exception() ->
?FORALL(Cmds, parallel_commands(?MODULE),
begin
{_History,_State,Result} = run_parallel_commands(?MODULE, Cmds),
Result =:= ok
end). does not do what it is supposed to because of an exception, but it does with the PR that fixes the issue reported in #241. Note the crucial difference in what I am suggesting and what this PR contains as test: Please supply the test case first that shows what goes wrong without this PR. |
Fix for #224