ocaml-semver / ocaml-api-watch

Libraries and tools to keep watch on you OCaml lib's API changes
ISC License
21 stars 15 forks source link

Port tests for changes to values to expect tests #38

Closed Siddhi-agg closed 8 months ago

Siddhi-agg commented 8 months ago

Aims to close #33. This is my initial work on the issue. I have opened this PR to record my progress and display it for ease in reviewing.

Siddhi-agg commented 8 months ago

@NathanReb I had a question. When I execute dune runtest I get the following error :

......................................{
                  desc = Tconstr (Path.Pident "int", [], ref []);
                  level = 0;
                  scope =0;
                  id = 1;
                };
Error: Cannot create values of the private type Types.transient_expr

Then, when I change the code to:

 let a = 0 in
  let signature1=
  [
   .... 
        type_manifest = Some {
                  desc = Tconstr (Path.Pident "int", [], ref []);
                  level = 0;
                  scope = Some a;
                  id = 1;
                };

It still gives the same error. Shouldn't changing the scope from 0 to Some a (0) change its type from int to int option and thus change the whole expression's type to type_expr?

harshey1103 commented 8 months ago

Hi @Siddhi-agg, I encountered a similar error to the one you mentioned. Since I am responsible for implementing the "types" part, I fixed it. You can refer here for the fix.

NathanReb commented 8 months ago

Indeed @Siddhi-agg, your error was that you were trying to build a value of a private type.

They are documented here if you want to take a look but in short, private types are kind of "read-only" types as in you can pattern match over their structure as if they were public but you cannot directly create a value of that type, you have to use API functions to do so just as if they were abstract.

E.g. if you have the following:

type t = private
  | A
  | B of int

val a : unit -> t
val b : int -> t

That means you can write the following code:

match t with
| A -> ...
| B i -> ...

but you cannot write:

let x = B 10

You will have to instead use the function provided above:

let x = b 10
Siddhi-agg commented 8 months ago

They are documented here if you want to take a look but in short, private types are kind of "read-only" types as in you can pattern match over their structure as if they were public but you cannot directly create a value of that type, you have to use API functions to do so just as if they were abstract.

Yeah, I read that and used create function from to get a declare a type_expr. @harshey1103 's implementation helped a lot in that. Thanks :)

I am currently working on why the diff_interface isn't detecting the modified values. I also noticed that @harshey1103 and @MungaSoftwiz are experiencing the same issue in their respective tests. Would be glad to help them if I solve the issue.

MungaSoftwiz commented 8 months ago

They are documented here if you want to take a look but in short, private types are kind of "read-only" types as in you can pattern match over their structure as if they were public but you cannot directly create a value of that type, you have to use API functions to do so just as if they were abstract.

Yeah, I read that and used create function from to get a declare a type_expr. @harshey1103 's implementation helped a lot in that. Thanks :)

I am currently working on why the diff_interface isn't detecting the modified values. I also noticed that @harshey1103 and @MungaSoftwiz are experiencing the same issue in their respective tests. Would be glad to help them if I solve the issue.

Yes that's an issue I have experienced too. From my PR I have used the expect tests as a placeholder for now. I've been trying to see how to navigate that issue better too. What's the output on your side please?

Siddhi-agg commented 8 months ago

What's the output on your side please?

@MungaSoftwiz On my side, I just get asked to change the expected output of the modify test to false:

diff --git a/_build/default/tests/api-watch-diff/test.ml b/_build/default/tests/api-watch-diff/test.ml.corrected
index 491825a..f3d351f 100644
--- a/_build/default/tests/api-watch-diff/test.ml
+++ b/_build/default/tests/api-watch-diff/test.ml.corrected
@@ -180,7 +180,7 @@ let%expect_test "modifying_a_value_test" =
       ~current:ref_signature
   in
   Format.printf "%b" result;
-  [%expect {|true|}]
+  [%expect {|false|}]

 (* Signature for remove_value.mli:
      > type t = int
MungaSoftwiz commented 8 months ago

What's the output on your side please?

@MungaSoftwiz On my side, I just get asked to change the expected output of the modify test to false:

diff --git a/_build/default/tests/api-watch-diff/test.ml b/_build/default/tests/api-watch-diff/test.ml.corrected
index 491825a..f3d351f 100644
--- a/_build/default/tests/api-watch-diff/test.ml
+++ b/_build/default/tests/api-watch-diff/test.ml.corrected
@@ -180,7 +180,7 @@ let%expect_test "modifying_a_value_test" =
       ~current:ref_signature
   in
   Format.printf "%b" result;
-  [%expect {|true|}]
+  [%expect {|false|}]

 (* Signature for remove_value.mli:
      > type t = int

Interesting @Siddhi-agg! It's surprising that a change in types in the modules outputs a more verbose exception. This bags the question, why is the change of a type in a module causing the particular backtrace compared to values?

Siddhi-agg commented 8 months ago

It also helped us figure out that in its current form it is neither a satisfying nor a scalable way to write tests and we need helpers to build signatures.

@NathanReb I agree. When I was building signatures for tests, even I thought that was a tedious and verbose process to do manually for the various tests this project may have in the future. Having a helper library to automate generating signatures would be nice! I would love to help in developing it if needed.