proper-testing / proper

PropEr: a QuickCheck-inspired property-based testing tool for Erlang
http://proper-testing.github.io
GNU General Public License v3.0
879 stars 168 forks source link

next_state is evaluated before checking postcondition in statem #65

Closed fredrikelinder closed 11 years ago

fredrikelinder commented 11 years ago

The expected (to me at least) flow in the execution phase of statem would be:

(1) check precondition (2) run command (3) check postcondition (4) update next_state

However, next_state is evaluated before checking the postcondition, which basically means the next_state cannot be certain that this update is legit.

The statem code uses the very same state value in both the call to postcondition and to next_state (as it should) => (3) and (4) can be reordered without affecting any existing proper clients.

But changing them so that (4) is not evaluated if (3) is false make next_state not have to check the postcondition as well.

kostis commented 11 years ago

You are quite unlikely to get an answer in this issue because it's not so clear (to me at least) what is the intent of your post. I suspect the initial developer of the statem code (@EiriniArvaniti) also does not know how to react on this one. For example,

In the first two cases, some concrete examples will help. If it's the third, a suggested patch for the code is the appropriate action here and some use cases showing the performance benefits.

fredrikelinder commented 11 years ago

Oh, thanks for clearing this us:

• nothing wrong with correctness. • I want a code change since the current implementation forces me to check the post condition in next_state/3 as well as in postcondition/3, making my next_state/3 implementation more complex. IMHO next_state/3 should not be called (in exec phase) unless postcondition/3 returns true.

/Fredrik

On 22 mar 2013, at 04:31, Kostis Sagonas notifications@github.com wrote:

You are quite unlikely to get an answer in this issue because it's not so clear (to me at least) what is the intent of your post. I suspect the initial developer of the statem code (@EiriniArvaniti) also does not know how to react on this one. For example,

Is there some correctness issue in what the code of statem currently does?

Is there some case where it's a problem (e.g. preventing you to do something you want)?

Is this a performance enhancing suggestion that you want us to adopt?

Or you just want to initiate discussion on the issues that are involved?

In the first two cases, some concrete examples will help. If it's the third, a suggested patch for the code is the appropriate action here and some use cases showing the performance benefits.

— Reply to this email directly or view it on GitHub.

eiriniar commented 11 years ago

The motivation for updating the state in all scenarios (i.e. regardless of the result of postcondition/3) is that in case the post-condition is wrong, it can be helpful for the user to know the state that the system has resulted in after violation of the post-condition. Keeping that in mind, we are not sure whether the change that you propose would be beneficial to other users. Could you maybe give us an example of a situation where the next_state/3 definition becomes too complex under the current implementation?

fredrikelinder commented 11 years ago

"too complex" is quite subjective. I prefer to have clean code so I forked proper to get the desired behavior.

https://github.com/fredrikelinder/proper

/Fredrik

On 25 mar 2013, at 06:56, Eirini Arvaniti notifications@github.com wrote:

The motivation for updating the state in all scenarios (i.e. regardless of the result of postcondition/3) is that in case the post-condition is wrong, it can be helpful for the user to know the state that the system has resulted in after violation of the post-condition. Keeping that in mind, we are not sure whether the change that you propose would be beneficial to other users. Could you maybe give us an example of a situation where the next_state/3 definition becomes too complex under the current implementation?

— Reply to this email directly or view it on GitHub.

kostis commented 11 years ago

Hej!

We are merely trying to understand the pros and cons to what you are proposing and (hopefully) appreciate any simplifications to user code that this change may offer. For this reason, an (ideally compelling) example will help.

You can of course always choose to go your own way, but please appreciate that we would very much like to be convinced that your proposed change is worthwhile and, more importantly, that it is a change that does not cause pain and sorrow to existing test suites and users.

fredrikelinder commented 11 years ago

Hello Kostis,

I would love to convince you, but the thing is I don't know how to. I don't know what you consider is acceptable or even good reasons to update proper.

If we can agree that "too complex" is whenever next_state/3 has to do the same job as postcondition/3 plus update the state (which IMHO is the only thing next_state/3 should do), then we're on the same page. :-)

Bonus question: if next_state/3 has to do what postcondition/3 does and then some, when why at all have postcondition/3?

/Fredrik

On Mon, Mar 25, 2013 at 2:09 PM, Kostis Sagonas notifications@github.comwrote:

Hej!

We are merely trying to understand the pros and cons to what you are proposing and (hopefully) appreciate any simplifications to user code that this change may offer. For this reason, an (ideally compelling) example will help.

You can of course always choose to go your own way, but please appreciate that we would very much like to be convinced that your proposed change is worthwhile and, more importantly, that it is a change that does not cause pain and sorrow to existing test suites and users.

— Reply to this email directly or view it on GitHubhttps://github.com/manopapad/proper/issues/65#issuecomment-15425743 .

eiriniar commented 11 years ago

Hi Fredrik,

as Kostis said we would be very happy to adopt changes that make the code cleaner and things simpler for users. Maybe it took me some extra time to see your point, i.e. why next_state/3 has to do the (additional) job of postcondition/3 under the current implementation. Does this happen for catching exceptions that might be raised in case of a state update under invalid postcondition?

fredrikelinder commented 11 years ago

Hello Eirini,

:-)

I do have a "try {ok, apply(M, F, A)} catch C:E -> {C, E} end"-wrapper in my symbolic calls to handle non-local returns. (Does this seems an ok solution to you?)

But the problem arises whenever you cannot or when it's in-feasible to update the state with what you get back from a call to the code under test, especially when postcondition/3 would have caught this.

/Fredrik

On 26 mar 2013, at 07:00, Eirini Arvaniti notifications@github.com wrote:

Hi Fredrik,

as Kostis said we would be very happy to adopt changes that make the code cleaner and things simpler for users. Maybe it took me some extra time to see your point, i.e. why next_state/3 has to do the (additional) job of postcondition/3 under the current implementation. Does this happen for catching exceptions that might be raised in case of a state update under invalid postcondition?

— Reply to this email directly or view it on GitHub.

eiriniar commented 11 years ago

Ok it is more clear now and we are actually convinced! Would you mind submitting a pull request for the proposed change?

fredrikelinder commented 11 years ago

Thank you :-)

I've created a pull request

/Fredrik

On Tue, Mar 26, 2013 at 2:36 PM, Eirini Arvaniti notifications@github.comwrote:

Ok it is more clear now and we actually convinced! Would you mind submitting a pull request for the proposed change?

— Reply to this email directly or view it on GitHubhttps://github.com/manopapad/proper/issues/65#issuecomment-15490229 .

kostis commented 11 years ago

Closed by pulling request #67.