Test_lib.require is broken

[vanyamil]

File : src/grader/Test_lib.ml, lines 704-711.

Issue : the function "require" is supposed to compare the incoming expressions "n" to a given expression "expr" and return a success on the first time n is equal to expr. However, there are no comparisons made in that function; instead, it returns the success effectively on the first run of the function, and then the "already" flag stays on.

[yurug]

@OCamlPro-Couderc, can you look at this, please?

[lyrm]

My mistake, the documentation is wrong (there is a correct version for require_syntax and not for require_expr and require). I just opened a pull request with a correction.

[vanyamil]

Hi Lyrm,

Your pull request doesn't seem to correct any part of the require function, would that be someone else correcting it?

Thanks.

[lyrm]

I don't think the function is wrong, I think it is just that it does not what you expect it to do. However, the name may not be very appropriate, I will agree with that.

I can't find a good example to use it but an example for require_syntax that does almost the same thing: you can add the optional argument ~on_open: require_syntax "open" to check that the syntax open is used at least one (you obviously can also use require to do that but you will need to write the appropriate converter from Parsetree.open_description to string).

[vanyamil]

I see. From my understanding, all require (and for that matter, require_syntax) does is still deliver a success on first call of that function? That's why the ~on_open: use works, because the first use coincides with the first call to the open command?

What would be the way to require a function use in the code then? I am looking to make sure that a certain helper function is used in the graded function.

[picdc]

There might be 2 solutions:

• If the helper function is declared in prepare.ml, you embed a reference that counts the how many times it has been applied, i.e.

let count_f = ref 0

let f x y =
    incr count_f;
    ...

Then you can simply check its content is different than 0 (using test_ref). Since the function is contained in prepare.ml, its exact implementation is unknown by the student. But it needs a bit of preparation (see #72)

• Another solution is to use ast_check_structure with

~on_expression:(function 
  { descr = (Pexp_apply ({ descr = (Pexp_ident "f" )}, _) } -> (* Success *)

The second solution seems cleaner, but you won't track the function usage if it has been aliased. If the student aliases the function and never use it, the check will still succeed. But I admit it might be a bit overkill to think about functions aliasing.

[adhameer]

For posterity, I'd like to post the solution we went with here (since the suggested one is missing some record field names, etc.):

(* ------------- Parse Tree Helpers -------------  *)

let name_of_ident ident = String.concat "." (Longident.flatten ident)

let function_app_checker (f : string) =
  let open Parsetree in
  let found = ref false in
  let checker =
    (function
      | { pexp_desc = Pexp_apply ({pexp_desc = Pexp_ident {Location.txt; _}}, _); _} ->
        if (name_of_ident txt) = f then found := true; []
      | _ -> [])
  in
  (found, checker)

(* ------------------- Tester -------------------  *)
let test_exercise =
  let (found_f, f_checker) = function_app_checker "f" in
  Section (
    find_binding code_ast "test" @@ fun expr ->
      let checked_ast =
        ast_check_expr
        ~on_expression: f_checker
        expr
      in
    (Message ([Text "Checking that"; Code "f"; Text "was used in"; Code "test"], Informative))
    ::
    (if !found_f then
      Message ([Text "Found use of "; Code "f"; Text "in"; Code "test"], Success 1)
      ::
      [] (* rest of the testing code, which should only be executed on success, goes here *)
     else
      [Message ([Text "You must call"; Code "f"; Text "in your"; Code "test"; Text "function"], Failure)])

[yurug]

Should we improve the documentation?

[adhameer]

I'm very unclear on how require, require_expr, and require_syntax are actually meant to be used, even after lyrm's PR.

[adhameer]

Okay, I now understand how they're used, but they don't actually seem to be that useful since they don't provide a way of checking if the required expression was found, so you can't produce a failure message if the required expression is not used.