-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add expect tests for ppx #474
Conversation
The tests look good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect it to still work on Windows, because we are testing under Cygwin, so diff
is available.
test/ppx_expect/main.ml
Outdated
@@ -0,0 +1,78 @@ | |||
let test_directory = "cases" | |||
let test_cases = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary, but I think it would be nice to get the test list by listing the directory.
Oh, and thanks for being so thorough as to make these tests and PR them :) |
Good point. The diffing seems to work, but it looks like there's a problem with the ppx extension on the Windows CI:
Any idea what this could be about? |
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. |
And looking at it even closer, it looks like it might be problem with packaging of 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? |
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:
I'd suggest to skip these tests on 4.02.3, but alternatively we could also sanitise the output. |
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. |
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. |
Also, for skipping the test on Windows, the best recommendation I can give is to use Lwt's little testing framework, and wrap each Here is some similar code: Lines 51 to 55 in d9ed54c
(This doesn't detect Cygwin though, I forgot what the exact condition for checking for Cygwin is. I think module |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to squashing, but I can squash-merge in GitHub, no need to spend any time on that unless you really want to :)
test/ppx_expect/main.ml
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 5.00 will wrongly fail this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Note that we'll have a similiar problem in
Line 100 in b52e211
let supports_o3 = major >= 4 && minor >= 3 in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, and thanks for noting. I'll push to master
to fix that.
The tests can't find Output looks like this:
We should probably change |
...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.
I worked on it in branch In the meantime, I disabled the PPX tests temporarily. See the commit linked above. |
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