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

Improve failure message #202

Merged
merged 7 commits into from
Mar 14, 2024
Merged

Conversation

n-osborne
Copy link
Collaborator

@n-osborne n-osborne commented Feb 15, 2024

This PR improve the state of #188

This PR is based on #197, please consider only the last 7 commits.

Basically, this PR makes postcond return the reason of the failure rather than just a boolean, so that we can print the Gospel terms being violated back to the user in case of failing test.

This lead to give another implementation of the STM_Sequential.Make functor in a new Ortac_runtime_qcheck_stm package to use this new postcond function and build and pass to QCheck the new failure message.

@n-osborne
Copy link
Collaborator Author

For demo purpose, I have pushed this branch with a failing test.

You can then see (with colours):

λ dune build @launchtests
File "plugins/qcheck-stm/test/dune.inc", line 49, characters 0-75:
49 | (rule
50 |  (alias launchtests)
51 |  (action
52 |   (run %{dep:array_stm_tests.exe} -v)))
random seed: 476198396
generated error fail pass / total     time test name
[✗]    1    0    1    0 / 1000     0.0s Array STM tests (generating)

--- Failure --------------------------------------------------------------------

Test Array STM tests failed (7 shrink steps):

   length


+++ Messages ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Messages for test Array STM tests:

Gospel specification violation in function length

  File "array.mli", line 7, characters 12-18:
    i = 42
  
  File "array.mli", line 8, characters 12-26:
    i = t.size + 1
  
when executing the following sequence of operations:

  length : 16

================================================================================
failure (1 tests failed, 0 tests errored, ran 1 tests)

Copy link
Contributor

@shym shym left a comment

Choose a reason for hiding this comment

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

Nice work, congratulations!
I confess that I didn’t carefully review the generated code 😅 but this looks all good to me.

Three small questions/remarks:

  • I assume you went with (++) to get it infix in the generated code? I think I would prefer the fully-qualified call, just like what is done for all the Gospel operators. And I would suggest picking a different one, to avoid the confusion with Gospelstdlib.(++).
  • This is really quibbling: I noticed various uses of @\n, which is better avoided according to Format’s documentation. Did you try alternatives (thinking it might make the generated output a bit more robust)?

@@ -1,3 +1,9 @@
val term_printer : string -> Ppxlib.Location.t -> Gospel.Tterm.term -> string
(** [term_printer text global_loc term] fetch the initial text representation of
[term] providede that [text] is the specification's text and [global_loc]
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: “providede“

@n-osborne
Copy link
Collaborator Author

Thanks!

  • I assume you went with (++) to get it infix in the generated code? I think I would prefer the fully-qualified call, just like what is done for all the Gospel operators. And I would suggest picking a different one, to avoid the confusion with Gospelstdlib.(++).

Yes, I went for the infix operator to avoid things like (append (append x y) z) but the risk of confusion with another function is a good reason to not do that. And if we don't have the infix notation because we qualify (which is reasonable), no reason to not adopt append.

  • This is really quibbling: I noticed various uses of @\n, which is better avoided according to Format’s documentation. Did you try alternatives (thinking it might make the generated output a bit more robust)?

This is the end-of-line that is mostly used in the project I think. Maybe we can change it with consistency in the whole repo if we find a better candidate.

@n-osborne n-osborne force-pushed the failure-message branch 2 times, most recently from 18cff6b to ec853a5 Compare March 14, 2024 07:54
This will be needed in the following development
Add the textual representation of the Gospel term for future display in
case of test failure.
The runtime reimplement the QCheckSTM.Sequential.Make functor to use new
postcond function. It also implement how the failure message is printed
and the append function for postcond.
The Spec module contains a dummy postcond function, always returning
true. The new postcond function return an `Ortac_runtime.report option`.
Checking one term return a value of such a type, with a singleton for
the list. Results of checking several terms will be combine using an
append function defined in the runtime.
@n-osborne
Copy link
Collaborator Author

CI is green, merging.
Thanks for the review @shym :-)

@n-osborne n-osborne merged commit 3cf6833 into ocaml-gospel:main Mar 14, 2024
3 checks passed
@n-osborne n-osborne deleted the failure-message branch March 14, 2024 10:23
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.

2 participants