Add test helper library for expect tests

[MungaSoftwiz]

closes #35 Module attemps to:

1. Parse the .mli content
2. Create a lexbuf to prepare for parsing and tokenizing
3. Take the parsed interface and generates a Typedtree of the AST
4. Uses Typemod for type checking
5. Retrieve Types.signature from type checking environment

[MungaSoftwiz]

This is my initial draft of the specifications @NathanReb @shonfeder . Please let me know where I can improve. I am yet to figure out a way of extracting the Types.signature from the Typedtree. For now I have just used signature as a placeholder

I have also used Compmisc to avoid binding of types and values with Env

[NathanReb]

I didn't pay attention initially but this should live in a separate, test only library. tests/test_helpers/ is probably a good place to move this.

You'll need to add a dune file there as well, with a library stanza without public name as we don't want to expose this, it's for internal use only.

[MungaSoftwiz]

This is looking good as a first draft!

As you can see here, Typemod.type_interface returns a Typedtree.signature. Now if you look at Typedtree.signature's definition here you can see it's a record with a sig_type : Types.signature field, which is the type we are looking for. Hopefully that should be the right way to build our signatures.

To figure out how to get your way with OCaml, "just" look for functions that produce the type you are looking for. Sometimes you'll need a series of function calls to get there but looking for one's return type can help you figure out the next and so on.

Getting a value of the right type is usually 90% of the work.

Once you dealt with the few suggestions I made, could you try out it out by refactoring one of the test @Siddhi-agg added in #38 ?

Yes definitely! I'm currently working on the suggested changes and would test. I also agree with you that getting the value of the right type is actually the most work for writing OCaml code.

[Siddhi-agg]

Once you dealt with the few suggestions I made, could you try out it out by refactoring one of the test @Siddhi-agg added in #38 ?

@MungaSoftwiz If you need any assistance in understanding or refactoring the tests from #38 , I would be happy to help!

[MungaSoftwiz]

Once you dealt with the few suggestions I made, could you try out it out by refactoring one of the test @Siddhi-agg added in #38 ?

@MungaSoftwiz If you need any assistance in understanding or refactoring the tests from #38 , I would be happy to help!

Hey @Siddhi-agg , thanks for reaching out! I appreciate the offer to help with the tests from #38 . I've been looking at them myself recently, and it would be great to collaborate on refactoring them once I am done with the main code refactor.

[Siddhi-agg]

Hey @Siddhi-agg , thanks for reaching out! I appreciate the offer to help with the tests from #38 . I've been looking at them myself recently, and it would be great to collaborate on refactoring them once I am done with the main code refactor.

Sure! Let me know when you are done with the main code and we can work together for the refactoring of tests.

[MungaSoftwiz]

Hello, @NathanReb and @shonfeder. Managed to work on generating the Type.signature dynamically from the type environment where the module now build without breaking. I did tests with the provided one in #35 and realised an exception was not being handled correctly.

The exception relates to an EnvError(_) which after following the backtrace, I believe is from our environment. Please let me know where we can improve. Here is the exception output:

@@ -11,4 +11,18 @@ val g : t -> bool
   let current = Test_helpers.compile_interface modified_interface in
   let result = Api_watch_diff.diff_interface ~reference ~current in
   Format.printf "%b" result;
-  [%expect {|false|}]
+  [%expect.unreachable]
+[@@expect.uncaught_exn {|
+  (* CR expect_test_collector: This test expectation appears to contain a backtrace.
+     This is strongly discouraged as backtraces are fragile.
+     Please change this test to not include a backtrace. *)
+
+  ("Env.Error(_)")
+  Raised at Env.error in file "typing/env.ml", line 672, characters 16-33
+  Called from Env.lookup_module_path in file "typing/env.ml", line 3057, characters 12-62
+  Called from Typemod.type_open_ in file "typing/typemod.ml", line 110, characters 13-71
+  Called from Typemod.initial_env.open_module in file "typing/typemod.ml", line 127, characters 12-51
+  Called from Typemod.initial_env in file "typing/typemod.ml", line 162, characters 9-26
+  Called from Test_helpers.generate_signature in file "tests/test_helpers/test_helpers.ml", line 8, characters 20-43
+  Called from Test__Test_with_helper.(fun) in file "tests/api-watch-diff/test_with_helper.ml", line 10, characters 18-62
+  Called from Expect_test_collector.Make.Instance_io.exec in file "collector/expect_test_collector.ml", line 234, characters 12-19 |}]

[Siddhi-agg]

Hi @MungaSoftwiz, this backtrace issue seems similar to the one you encountered in #37. Maybe you can try debugging it using the process described by @shonfeder in https://github.com/NathanReb/ocaml-api-watch/pull/37#issuecomment-2005457567.

[MungaSoftwiz]

Hi @MungaSoftwiz, this backtrace issue seems similar to the one you encountered in #37. Maybe you can try debugging it using the process described by @shonfeder in #37 (comment).

Absolutely! I've been looking at this exception today and I found out the error stems from the line below which internally performs type checking and generates a typedtree representation.

let typed_tree = Typemod.type_interface initial_env intf in

The exact error raised is by https://github.com/ocaml/ocaml/blob/trunk/typing/env.ml#L672. It suggests that there's an error when comparing two environment objects, in our case the typed environment and the .mli file.

This was the last call in our stack call Env.lookup_module_path before the exception. The code for the same is at https://github.com/ocaml/ocaml/blob/trunk/typing/env.mli#L220-L221.

So I think we'll have to carefully review our code that constructs and sets up the environment since the module path is not being found or something. Currently working on resolving the issue.

[Siddhi-agg]

Absolutely! I've been looking at this exception today and I found out the error stems from the line below which internally performs type checking and generates a typedtree representation.

let typed_tree = Typemod.type_interface initial_env intf in

@MungaSoftwiz Actually, I think the error traces back to the line as given in the exception by Called from Test_helpers.generate_signature in file "tests/test_helpers/test_helpers.ml", line 8, characters 20-43 :

 let initial_env = Compmisc.initial_env () in

I looked at the Compmisc.initial_env () (https://github.com/ocaml/ocaml/blob/trunk/driver/compmisc.ml#L72-L75) and found out it makes a function call to the Typemod.initial_env which in turn calls the type_open_ (https://github.com/ocaml/ocaml/blob/trunk/typing/typemod.ml#L130) function defined in Typemod and that function uses Env.module_lookup_path (https://github.com/ocaml/ocaml/blob/trunk/typing/typemod.ml#L113).

I may be wrong in my analysis, but just wanted to share my findings with you. I am currently looking through the code to identify what is the specific error.

[MungaSoftwiz]

Thanks for the findings @Siddhi-agg. This is closer to what we're trying to achieve. So I just read about it and saw that as much as both Compmisc.initial_env and Typemod.initial_env create initial environments, they have different configurations and use cases.

• Compmisc.initial_env: initializes an environment suitable for compilation tasks so mostly used for compiling and processing OCaml source code.
• Typemod.initial_env: initializes an environment for type-checking operations so specifically designed for type-checking modules and their contents.

So on changing the environment initialization to Typemod.initial_env like before, I get a more graceful exception which indicates a change not being detected.

I must admit that we're not there yet, but I think we're on the right track. Exception currently raised:

--- a/_build/default/tests/api-watch-diff/test_with_helper.ml
+++ b/_build/default/tests/api-watch-diff/test_with_helper.ml.corrected
@@ -11,4 +11,4 @@ val g : t -> bool
   let current = Test_helpers.compile_interface modified_interface in
   let result = Api_watch_diff.diff_interface ~reference ~current in
   Format.printf "%b" result;
-  [%expect {|false|}]
+  [%expect {|true|}]

[Siddhi-agg]

I must admit that we're not there yet, but I think we're on the right track. Exception currently raised:

--- a/_build/default/tests/api-watch-diff/test_with_helper.ml
+++ b/_build/default/tests/api-watch-diff/test_with_helper.ml.corrected
@@ -11,4 +11,4 @@ val g : t -> bool
   let current = Test_helpers.compile_interface modified_interface in
   let result = Api_watch_diff.diff_interface ~reference ~current in
   Format.printf "%b" result;
-  [%expect {|false|}]
+  [%expect {|true|}]

@MungaSoftwiz Indeed! It looks like we are in the right direction. Also, can you share the test you are creating here?

I did tests with the provided one in #35 and realised an exception was not being handled correctly.

If it is the one in #35 as you mentioned here, shouldn't the expected output be true since the current and modified interfaces are different?

[MungaSoftwiz]

@MungaSoftwiz Indeed! It looks like we are in the right direction. Also, can you share the test you are creating here?

I did tests with the provided one in #35 and realised an exception was not being handled correctly.

If it is the one in #35 as you mentioned here, shouldn't the expected output be true since the current and modified interfaces are different?

Hey @Siddhi-agg the test in the one in #35, you just have to change the interface in let modified_interface = interface ^ to let modified_interface =ref_interface ^. We are checking if the interfaces are similar. So true equates to similarity and false equates to a difference in the interfaces.

[Siddhi-agg]

Hey @Siddhi-agg the test in the one in #35, you just have to change the interface in let modified_interface = interface ^ to let modified_interface =ref_interface ^

@MungaSoftwiz Yes, I assumed that change. But if we look at the Api_watch_diff.diff_interface usage in bin/api_diff.ml, it says false corresponds to similarity and true to difference. (https://github.com/NathanReb/ocaml-api-watch/blob/main/bin/api_diff.ml#L5-L9)

Also in the Api_watch_diff.diff_interface here we are matching false to the case when both the signatures are similar (sig_1 can be coerced into sig_2 and sig_2 can be coerced into sig_1).

[MungaSoftwiz]

Also in the Api_watch_diff.diff_interface here we are matching false to the case when both the signatures are similar (sig_1 can be coerced into sig_2 and sig_2 can be coerced into sig_1).

Oh yes! I totally missed that, thank you. Then this code is mostly correct I think. We just have to do some tests with your value expect tests as instructed.

[NathanReb]

Indeed Compmisc.initial_env is bound to fail since we do not properly initialize the paths needed for OCaml to know where the standard library is.

For now it's not strictly required because we use very simple interfaces but we might want to fix this in the future. We should be able to look up the right environment variable set by dune to find it. Let's not bother with this for now if it works with Typemod.initial_env!