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

Lwt sequence tests #536

Merged
merged 9 commits into from
Jan 13, 2018
Merged

Lwt sequence tests #536

merged 9 commits into from
Jan 13, 2018

Conversation

cedlemo
Copy link
Contributor

@cedlemo cedlemo commented Jan 9, 2018

@aantron, Just a few tests for you to see if it correspond to what you want.

@aantron
Copy link
Collaborator

aantron commented Jan 9, 2018

They look good :)

@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 10, 2018

@aantron, I have tested all the Lwt_sequence functions but I have not been able to run the coverage locally in order to verify the coverage.

make coverage
jbuilder clean
find . -name '.merlin' | xargs rm -f
rm -fr doc/api
rm -f src/jbuild-ignore src/unix/lwt_config src/core/flambda.flag
for TEST in `ls -d test/packaging/*/*` ; \
do \
    make -wC $TEST clean ; \
done
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/core »
jbuilder clean --root .
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/core »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/log »
jbuilder clean --root .
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/log »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/ppx »
jbuilder clean --root .
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/ppx »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/ppx_ppxopt »
jbuilder clean --root .
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/ppx_ppxopt »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/preemptive »
jbuilder clean --root .
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/preemptive »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/unix »
jbuilder clean --root .
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/jbuilder/unix »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/camlp4 »
rm -f *.cm* *.o a.out
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/camlp4 »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/core »
rm -f *.cm* *.o a.out
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/core »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/ppx »
rm -f *.cm* *.o a.out
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/ppx »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/preemptive »
rm -f *.cm* *.o a.out
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/preemptive »
make[1] : on entre dans le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/unix »
rm -f *.cm* *.o a.out
make[1] : on quitte le répertoire « /home/cedlemo/Projets/OCaml/lwt/test/packaging/ocamlfind/unix »
rm -rf _coverage/
BISECT_ENABLE=yes jbuilder runtest --dev
Multiple rules generated for _build/default/src/camlp4/pa_lwt.odoc
make: *** [Makefile:109: coverage] Error 1

@aantron
Copy link
Collaborator

aantron commented Jan 10, 2018

Thanks, I'm looking into the coverage issue.

@aantron
Copy link
Collaborator

aantron commented Jan 10, 2018

@cedlemo, I fixed the coverage build in master, see 9a3d331. You should be able to pick it up by rebasing, hope it's not too much of a hassle. Thanks for letting me know, and sorry about that. Should probably build coverage in CI.

@cedlemo cedlemo force-pushed the lwt_sequence_tests branch from f58e41e to e3e3707 Compare January 10, 2018 15:25
@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 10, 2018

@aantron,

I have rebased and pushed everything.

@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 10, 2018

96% coverage. It remains Lwt_sequence.set to test and the coverage report seems to mark some loop functions, I guess it is some false positive.

Copy link
Collaborator

@aantron aantron left a comment

Choose a reason for hiding this comment

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

These look good. We can merge them now, however coverage suggests that a couple more tests can be added. If you'd like to do that, please go ahead – either way, let me know when you think it's ready for merge :)


test "fold_l" begin fun () ->
let sum = Lwt_sequence.fold_l (fun v e -> v + e) (filled_sequence ()) 0 in
Lwt.return (sum = 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally a nit, but I would change + to * (easiest) or change the sequence elements. Starting with zero and checking for zero could fail to catch a mistake where we accidentally forget to call f at all, and just propagate the accumulator during fold. Not that anyone is actually going to edit Lwt_sequence and do that, so feel free not to fix it :)

@aantron
Copy link
Collaborator

aantron commented Jan 10, 2018

Oops, cross-posted.

the coverage report seems to mark some loop functions, I guess it is some false positive.

This is probably because it is reporting coverage of Lwt_sequence from running the whole Lwt test suite, not just the Lwt_sequence tests, and some of the other tests transitively depend on Lwt_sequence. I actually manually removed all the other tests when looking at this PR locally, and the coverage was the same – so I'm not sure if it's really a false positive.

Manually editing the test suite to filter it is pretty tedious, we have an issue in Bisect_ppx to somehow support filtering in the report aantron/bisect_ppx#131.

@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 10, 2018

@aantron, I have added the missing test and improved the Lwt_sequence.fold_* function tests.

@aantron
Copy link
Collaborator

aantron commented Jan 10, 2018

Thanks. @cedlemo, the remaining uncovered lines are triggered in this circumstance: #405 (comment). Do you think you could get them covered also? As you can see from that issue, it was an actual bug that had to be fixed, so it would be nice to test it.

Of course, please correct me if my memory is hazy.

And, you don't have to add those tests. I'm happy to merge the PR as is :)

@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 10, 2018

Do you think you could get them covered also?

@aantron, I will look at this tomorrow.

@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 11, 2018

@aantron, I tried to find a way to (re)create the possible error mentionned in #405 with the following example :

edit TL;DR;

I finally find an example :

let fold_with_current_prev_and_next_removal () =
  let s = create_filled_sequence () in
  fold_r (fun v acc ->
    (if v = 3 then
      let n = find_node_r (fun v' -> v' = 3) s in
      let _ = remove n in
      let n = find_node_r (fun v' -> v' = 4) s in
      let _ = remove n in
      let n = find_node_r (fun v' -> v' = 2) s in
      ignore(remove n));
      print_endline (string_of_int v); acc * v) s 1

output:

fold with current 3 prev 4 and next 2 removal
5
4
3
5
1
exception Empty

type 'a t = {
  mutable prev : 'a t;
  mutable next : 'a t;
}

type 'a node = {
  mutable node_prev : 'a t;
  mutable node_next : 'a t;
  mutable node_data : 'a;
  mutable node_active : bool;
}

external seq_of_node : 'a node -> 'a t = "%identity"
external node_of_seq : 'a t -> 'a node = "%identity"

let remove node =
  if node.node_active then begin
    node.node_active <- false;
    let seq = seq_of_node node in
    seq.prev.next <- seq.next;
    seq.next.prev <- seq.prev
  end

let create () =
  let rec seq = { prev = seq; next = seq } in
  seq

let is_empty seq = seq.next == seq

let add_r data seq =
  let node = { node_prev = seq.prev; node_next = seq; node_data = data; node_active = true } in
  seq.prev.next <- seq_of_node node;
  seq.prev <- seq_of_node node;
  node

let fold_r f seq acc =
  let rec loop curr acc =
    if curr == seq then
      acc
    else
      let node = node_of_seq curr in
      if node.node_active then
        loop node.node_prev (f node.node_data acc)
      else
        loop node.node_next acc
  in
  loop seq.prev acc

let find_node_r f seq =
  let rec loop curr =
    if curr != seq then
      let node = node_of_seq curr in
      if node.node_active then
        if f node.node_data then
          node
        else
          loop node.node_prev
      else
        loop node.node_prev
    else
      raise Not_found
  in
  loop seq.prev

let create_filled_sequence () =
  let s = create () in
  let _ = add_r 1 s in
  let _ = add_r 2 s in
  let _ = add_r 3 s in
  let _ = add_r 4 s in
  let _ = add_r 5 s in
  s

let simple_fold () =
  let s = create_filled_sequence () in
  fold_r (fun v acc -> print_endline (string_of_int v); acc * v) s 1

let fold_with_current_removal () =
  let s = create_filled_sequence () in
  fold_r (fun v acc ->
    (if v = 2 then
      let n = find_node_r (fun v' -> v' = 2) s in
      ignore(remove n)
     );
     print_endline (string_of_int v); acc * v) s 1

let fold_with_prev_removal () =
  let s = create_filled_sequence () in
  fold_r (fun v acc ->
    (if v = 2 then
      let n = find_node_r (fun v' -> v' = 3) s in
      ignore(remove n));
      print_endline (string_of_int v); acc * v) s 1

let fold_with_current_and_prev_removal () =
  let s = create_filled_sequence () in
  fold_r (fun v acc ->
    (if v = 2 then
      let n = find_node_r (fun v' -> v' = 2) s in
      let _ = remove n in
      let n = find_node_r (fun v' -> v' = 3) s in
      ignore(remove n)
      );
      print_endline (string_of_int v); acc * v) s 1

let fold_with_next_removal () =
  let s = create_filled_sequence () in
  fold_r (fun v acc ->
    (if v = 3 then
      let n = find_node_r (fun v' -> v' = 2) s in
      ignore(remove n));
      print_endline (string_of_int v); acc * v) s 1

let fold_with_current_and_next_removal () =
  let s = create_filled_sequence () in
  fold_r (fun v acc ->
    (if v = 3 then
      let n = find_node_r (fun v' -> v' = 2) s in
      let _ = remove n in
      let n = find_node_r (fun v' -> v' = 3) s in
      ignore(remove n));
      print_endline (string_of_int v); acc * v) s 1

let _ =
  let _ = print_endline "simple fold" in
  let _ = simple_fold () in
  let _ = print_endline "fold with current 2 removal" in
  let _ = fold_with_current_removal () in
  let _ = print_endline "fold with prev 3 removal" in
  let _ = fold_with_prev_removal () in
  let _ = print_endline "fold with current 2 and prev 3 removal" in
  let _ = fold_with_current_and_prev_removal () in
  let _ = print_endline "fold with next 2 removal" in
  let _ = fold_with_next_removal () in
  let _ = print_endline "fold with current 3 and next 2 removal" in
  ignore(fold_with_current_and_next_removal ())

the ouput is :

~/Projets/OCaml/tests
⟩ ocamlfind ocamlc -o lwt_fold_r_bad -linkpkg -g lwt_fold_r_bad.ml 

~/Projets/OCaml/tests
⟩ ./lwt_fold_r_bad 
simple fold
5
4
3
2
1
fold with current 2 removal
5
4
3
2
1
fold with prev 3 removal
5
4
3
2
1
fold with current 2 and prev 3 removal
5
4
3
2
1
fold with next 2 removal
5
4
3
1
fold with current 3 and next 2 removal
5
4
3
1

I dont see a problem here, have you got an idea on how I could trigger the error ?

@cedlemo cedlemo force-pushed the lwt_sequence_tests branch from 537d28b to 3a8d210 Compare January 11, 2018 17:25
@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 11, 2018

rebase done.

@aantron aantron merged commit 4f0dabc into ocsigen:master Jan 13, 2018
@aantron
Copy link
Collaborator

aantron commented Jan 13, 2018

Thanks :)

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