Add expect tests for ppx #474

copy · 2017-09-27T13:42:43Z

This adds expect tests for some of the ppx constructs. It uses some code from ppx_bisect's test helpers. It doesn't work on Windows, due to using diff.

cc @Drup @aantron

Drup · 2017-09-27T13:51:56Z

The tests look good!

aantron

I would expect it to still work on Windows, because we are testing under Cygwin, so diff is available.

aantron · 2017-09-27T14:06:21Z

test/ppx_expect/main.ml

@@ -0,0 +1,78 @@
+let test_directory = "cases"
+let test_cases = [


It's not necessary, but I think it would be nice to get the test list by listing the directory.

aantron · 2017-09-27T14:09:35Z

Oh, and thanks for being so thorough as to make these tests and PR them :)

copy · 2017-09-27T15:03:09Z

I would expect it to still work on Windows, because we are testing under Cygwin, so diff is available.

Good point. The diffing seems to work, but it looks like there's a problem with the ppx extension on the Windows CI:

# The system cannot find the path specified.
# > diff -au let_1.expect let_1.fixed:
# 
# --- let_1.expect	2017-09-27 13:49:32.561606300 +0000
# +++ let_1.fixed	2017-09-27 13:49:39.080199000 +0000
# @@ -1,3 +1,2 @@
# -File "./let_1.ml", line 6, characters 10-12:
# -Error: This pattern matches values of type unit
# -       but a pattern was expected which matches values of type int
# +File ".\let_1.ml", line 6, characters 6-9:
# +Uninterpreted extension 'lwt'.
#

Any idea what this could be about?

aantron · 2017-09-27T18:48:26Z

I haven't looked into it deeply at all, but given that the existing PPX test suite runs in Windows CI (example), I would guess that there is a problem with the details of how the expect tests are invoking the PPX.

aantron · 2017-09-27T18:52:40Z

And looking at it even closer, it looks like it might be problem with packaging of lwt.ppx, since the package is found by topfind, but that doesn't result in the PPX being loaded. I'm not sure if that's inherent to the environment the toplevel ends up running in. I suppose it isn't, because it works on Unix.

I would have to RDP into the build worker to debug this. Would you be willing to do that, or debug it on a Windows machine you have access to?

copy · 2017-09-27T19:56:03Z

I don't think I have the expertise with Windows to look into this.

There also seems to be a problem on 4.02.3, which is in how the compiler prints paths:

-File "./let_1.ml", line 6, characters 10-12:
+File "let_1.ml", line 6, characters 10-12:

I'd suggest to skip these tests on 4.02.3, but alternatively we could also sanitise the output.

aantron · 2017-09-27T20:00:06Z

It might be more productive to run ocamlfind explicitly instead of using the toplevel.

I'm fine with disabling the tests anywhere they don't work for now, though. I think we only really need them to work in one build row. There shouldn't be any differences with other build rows anyway, at least any difference caused by Lwt, and having them work anywhere is already an improvement over not having them at all.

aantron · 2017-09-27T20:04:59Z

Well, to be more precise, there shouldn't be any difference due to Lwt between systems for one compiler version. For different versions, though, the compiler could give us different locations to work with, that we then use for the generated locations. But this isn't that likely. I guess we won't have any testing on 4.02, but that's okay if we can't easily work around the output issue. If you'd like to sanitize the output, it would be welcome. Not testing on Windows is completely fine.

aantron · 2017-09-27T20:11:15Z

Also, for skipping the test on Windows, the best recommendation I can give is to use Lwt's little testing framework, and wrap each .ml file in a single generated test case, and combine all those into a test suite. Use the ~only_if optional argument to skip each case on Windows. This will make it clear in the output that the tests were skipped, and the output will be consistent with other skipped tests elsewhere in Lwt's test suites.

Here is some similar code:

lwt/test/unix/test_mcast.ml

Lines 51 to 55 in d9ed54c

 let test_mcast name join set_loop = 

 test name ~only_if:(fun () -> not Sys.win32) begin fun () -> 

 let should_timeout = not join || not set_loop in 

 let fd1 = Lwt_unix.(socket PF_INET SOCK_DGRAM 0) in 

 let fd2 = Lwt_unix.(socket PF_INET SOCK_DGRAM 0) in

(This doesn't detect Cygwin though, I forgot what the exact condition for checking for Cygwin is. I think module Sys will say :)).

copy · 2017-09-28T16:46:00Z

Okay, I updated the test to use Lwt's test suite (which makes more sense anyway), and to skip the test on Windows, cywin, and OCaml < 4.03.

Let me know if I should squash these commits together.

aantron

Yes to squashing, but I can squash-merge in GitHub, no need to spend any time on that unless you really want to :)

aantron · 2017-09-28T16:56:22Z

test/ppx_expect/main.ml

+ Sys.cygwin = false && Sys.win32 = false &&
+ (* 4.02.3 prints file paths differently *)
+ Scanf.sscanf Sys.ocaml_version "%u.%u"
+ (fun major minor -> major >= 4 && minor >= 3)


I think 5.00 will wrongly fail this test.

Good point. Note that we'll have a similiar problem in

lwt/src/util/configure.ml

Line 100 in b52e211

let supports_o3 = major >= 4 && minor >= 3 in

Good catch, and thanks for noting. I'll push to master to fix that.

@copy

Caught by @copy in #474.

aantron · 2017-09-28T22:22:21Z

Thanks for doing this @copy, and thanks @Drup for helping move this along and review it here and in #337.

aantron · 2017-10-14T12:49:47Z

The tests can't find lwt.ppx when lwt isn't installed through opam (which is typical for me during development). In other words, they have been running against an installed Lwt, not against the Lwt in the working tree. I think the "right" way to write the tests is as a bunch of small Jbuilder-ized programs, so that Jbuilder can find lwt.ppx for them in the working tree. For comparison, see these little programs here: https://github.com/ocsigen/lwt/tree/master/test/packaging/jbuilder

Output looks like this:

(8/11) Running test "match_4" from suite "ppx_expect"
No such package: lwt
No such package: lwt.ppx
Test "match_4" from suite "ppx_expect" failed. It raised: "Failure(\"> diff -au match_4.expect match_4.fixed:\\n\\n--- match_4.expect\\t2017-10-14 07:39:15.000000000 -0500\\n+++ match_4.fixed\\t2017-10-14 07:39:17.000000000 -0500\\n@@ -1,4 +1,2 @@\\n-File \\\"./match_4.ml\\\", line 12, characters 4-16:\\n-Error: This expression has type int Lwt.t\\n-       but an expression was expected of type unit Lwt.t\\n-       Type int is not compatible with type unit \\n+File \\\"./match_4.ml\\\", line 6, characters 8-11:\\n+Error: Uninterpreted extension 'lwt'.\\n\")".

We should probably change travish.sh so that it doesn't install Lwt before running the tests, if possible, to catch such conditions.

aantron · 2017-10-14T12:50:44Z

...aaaand I'll work on it :)

These tests are linking against an installed package lwt (and lwt.ppx), not the ones in the working tree. This has two effects: - It's misleading to developers. - It makes the tests fail spuriously when a developer doesn't have lwt installed in their opam switch. Related #474.

aantron · 2017-10-21T17:11:04Z

I worked on it in branch ppx-test-install-ordering last week, but eventually moved on to other things after reading a ton of Findlib code. EDIT: The OCAMLPATH variable wasn't working for packages loaded from the toplevel.

In the meantime, I disabled the PPX tests temporarily. See the commit linked above.

The tests were originally added in #474. Fixes #492.

Add expect tests for ppx errors

15ab560

aantron reviewed Sep 27, 2017

View reviewed changes

copy added 2 commits September 27, 2017 17:13

Find test files from directory instead of hard-coded list

5d6ba7d

Add -noinit to toplevel invocation

3c8049b

aantron mentioned this pull request Sep 28, 2017

Jenkins builds appearing as stuck #475

Closed

Integrate ppx_expect into lwt's test suite, skip on Windows and 4.02.3

63d8501

aantron reviewed Sep 28, 2017

View reviewed changes

Fix version comparison

5840e1a

aantron added a commit that referenced this pull request Sep 28, 2017

Configure: fix compiler version bound check

3abe1e2

Caught by @copy in #474.

aantron merged commit 20a51b6 into ocsigen:master Sep 28, 2017

aantron mentioned this pull request Oct 28, 2017

Fix the PPX expect tests #492

Closed

aantron mentioned this pull request Dec 19, 2017

Lwt 3.2.0 ocaml/opam-repository#11071

Merged

aantron added a commit that referenced this pull request Mar 25, 2018

Restore PPX expect tests

a1fc678

The tests were originally added in #474. Fixes #492.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expect tests for ppx #474

Add expect tests for ppx #474

copy commented Sep 27, 2017

Drup commented Sep 27, 2017

aantron left a comment

aantron Sep 27, 2017

aantron commented Sep 27, 2017

copy commented Sep 27, 2017

aantron commented Sep 27, 2017

aantron commented Sep 27, 2017

copy commented Sep 27, 2017

aantron commented Sep 27, 2017

aantron commented Sep 27, 2017

aantron commented Sep 27, 2017 •

edited

Loading

copy commented Sep 28, 2017

aantron left a comment

aantron Sep 28, 2017

copy Sep 28, 2017

aantron Sep 28, 2017

aantron commented Sep 28, 2017

aantron commented Oct 14, 2017

aantron commented Oct 14, 2017

aantron commented Oct 21, 2017 •

edited

Loading

		@@ -0,0 +1,78 @@
		let test_directory = "cases"
		let test_cases = [

Add expect tests for ppx #474

Add expect tests for ppx #474

Conversation

copy commented Sep 27, 2017

Drup commented Sep 27, 2017

aantron left a comment

Choose a reason for hiding this comment

aantron Sep 27, 2017

Choose a reason for hiding this comment

aantron commented Sep 27, 2017

copy commented Sep 27, 2017

aantron commented Sep 27, 2017

aantron commented Sep 27, 2017

copy commented Sep 27, 2017

aantron commented Sep 27, 2017

aantron commented Sep 27, 2017

aantron commented Sep 27, 2017 • edited Loading

copy commented Sep 28, 2017

aantron left a comment

Choose a reason for hiding this comment

aantron Sep 28, 2017

Choose a reason for hiding this comment

copy Sep 28, 2017

Choose a reason for hiding this comment

aantron Sep 28, 2017

Choose a reason for hiding this comment

aantron commented Sep 28, 2017

aantron commented Oct 14, 2017

aantron commented Oct 14, 2017

aantron commented Oct 21, 2017 • edited Loading

aantron commented Sep 27, 2017 •

edited

Loading

aantron commented Oct 21, 2017 •

edited

Loading