Port win-unicode test to ocamltest #1730
Conversation
|
Thanks @nojb! Will review ASAP!
|
|
@nojb could you please update .gitognore ?
If you don't mind, just amend the commit rather than adding one.
|
|
I have two general questions:
It seems the tests will run everywhere: is it on purpose that you didn't
guard them with the Config.windows_unicode stuff?
Second, the Makefile uses a number of flags to ocamlc and ocamlopt
that you don't use in the test block. Is that on purpose that you
omitted them?
|
nojb
force-pushed
the
win_unicode_ocamltests
branch
from
262fbc7
to
f225998
Compare
Done. |
|
Nicolás Ojeda Bär (2018/04/19 00:38 -0700):
> could you please update .gitognore ?
Done.
Great! Thanks!
|
No, I just didn't know how to do it. The test will only compile on Windows and will fail if Unicode is not enabled, so: how do I restrict a test to run only when |
No, I forgot to put them back; but in any case they were not important for the test. |
|
Nicolás Ojeda Bär (2018/04/19 07:48 +0000):
> Second, the Makefile uses a number of flags to ocamlc and ocamlopt
that you don't use in the test block. Is that on purpose that you
omitted them?
No, I forgot to put them back; but in any case they were not important
for the test.
OK, no problem. It's just that I try to make sure the porting is just a
porting and does not change anything to the tests if this is not
required by the porting itself.
|
|
Nicolás Ojeda Bär (2018/04/19 00:45 -0700):
> It seems the tests will run everywhere: is it on purpose that you didn't
guard them with the Config.windows_unicode stuff?
No, I just didn't know how to do it. The test will only compile on
Windows and will fail if Unicode is not enabled, so: how do I restrict
a test to run only when `Config.windows_unicode` is `true` ?
See https://github.com/shindere/ocaml/tree/win-unicode-test
I'd suggest that you cherry-pick this as the first commit of your
branch. It adds a "windows-unicode" test. You may have to amend the
commit, I didn't test it,it was mainly to show you how it goes.
|
nojb
force-pushed
the
win_unicode_ocamltests
branch
3 times, most recently
from
d90ce72
to
8b20939
Compare
Done. Seems to work well, except the comparison of the reference file |
|
Nicolás Ojeda Bär (2018/04/19 02:05 -0700):
> I'd suggest that you cherry-pick this as the first commit of your
branch. It adds a "windows-unicode" test. You may have to amend the
commit, I didn't test it,it was mainly to show you how it goes.
Done.
Great!
Seems to work well, except the comparison of the reference file
`win-unicode/exec_tests.reference`. Does `ocamltest` ignore
end-of-line differences when comparing? It should.
I seem to remember that that part had been implemented by @dra27
but can not find in which commit it was. :(
In any case this test used to
be disabled because it could not be run consistently in the CI,
Any idea what the problem was, exactly?
so we may have to disable it anyway
It'd be okay to do so if we are not able to fix it, I think. The main
purpose here is to port the tests. If we can fix things that didn't work
that's very fine, but it's not a duty either. We just have to make sure
we don't make things worse while porting, IMO.
(but let's see if problems persist when
running it under `ocamltest`).
Many thanks for looking into this, anyway.
|
|
This has nothing to do with line-endings - the files really are different! |
|
There is at least a bug in ocamltest's detection for process termination (GPR for that shortly) and this test is presently sensitive to whether ocaml is installed or not - but I'm not sure yet whether that's the Makefile which is at fault or ocamltest! |
|
The weird comparison error message is now fixed - basically, at the point at which ocamltest was comparing the files, the @nojb - have you tested this on msvc or just mingw? It looks like the symlink_tests isn't linking the C stubs and I wondered if this might be a .o/.obj thing in ocamltest, but I haven't looked in detail. I'm also seeing errors loading unix.cma unless I have ocaml preinstalled which I think means something to do with the old |
|
Thanks for the help @dra27! I haven't yet looked at this again, but plan to do it between today and tomorrow; will keep my eyes open. |
|
David Allsopp (2018/04/26 12:39 +0000):
@nojb - have you tested this on msvc or just mingw? It looks like the
symlink_tests isn't linking the C stubs and I wondered if this might
be a .o/.obj thing in ocamltest, but I haven't looked in detail. I'm
also seeing errors loading unix.cma unless I have ocaml preinstalled
which I think means something to do with the old `SET_LD_PATH` may be
required - but there must be other tests which already need to do this
(again, I just haven't looked yet).
Yeah I thought I did that carefully and AFAIK there's no OCaml installed
e.g. on Inria's CI machines. HOw about AppVeyor? Is it possible that
these have a preinstalled OCaml?
|
|
@shindere - AppVeyor doesn't have an OCaml installed - what I was guessing was that @nojb does, as he seemed to be only talking about the error with the file comparison, not the other errors trying to run the tests! Is there something which should be in the ML file to ensure that the correct directory is available to ocamlrun for finding |
|
David Allsopp (2018/04/26 13:35 +0000):
@shindere - AppVeyor doesn't have an OCaml installed - what I was
guessing was that @nojb does, as he seemed to be only talking about
the error with the file comparison, not the other errors trying to run
the tests! Is there something which should be in the ML file to ensure
that the correct directory is available to ocamlrun for finding
`dllunix.dll`?
Yeah, "include unix". That will include either unix or win32unix.
So if "include unix" things do not work properly, then I think that's a
bug that needs to be fixed. :)
BTW: now that most of the tests have been ported to ocamltest, I think
people who have already written tests with the makefiles can look how
the tests they know look, in the new style. That may be a source of
inspiration until proper documentation has b een written.
|
nojb
force-pushed
the
win_unicode_ocamltests
branch
2 times, most recently
from
ce4bbc4
to
596b5a6
Compare
nojb
force-pushed
the
win_unicode_ocamltests
branch
from
596b5a6
to
54a37ba
Compare
| ** setup-ocamlc.byte-build-env | ||
| program = "${test_build_directory}/mltest.byte" | ||
| *** ocamlc.byte | ||
| program = "${test_build_directory}/print.exe" |
There was a problem hiding this comment.
@shindere This auxiliary program needs to be compiled with the unix library, how do I do that?
|
Nicolás Ojeda Bär (2018/05/09 02:06 -0700):
@shindere This auxiliary program needs to be compiled with the `unix`
library, how do I do that?
Are you saying that the "include unix" at the beginning of the test
block is not enough for that to work? I'd have guessed it would have
been enough, no?
|
No, it does not work. The program compiles fine, but the bytecode version of the helper program |
|
Nicolás Ojeda Bär (2018/05/09 02:42 -0700):
> Are you saying that the "include unix" at the beginning of the test
block is not enough for that to work? I'd have guessed it would have
been enough, no?
No, it does not work. The program compiles fine, but the bytecode
version of the helper program `print.exe` fails to find the `unix`
DLL. (One needs to set `CAML_LD_LIBRARY_PATH` I guess).
How is the auxiliary program run? Is it run by the main test program? If
so, how is that main test program run? By a "run" ocamltest action, or
by a "script" one?
|
nojb
force-pushed
the
win_unicode_ocamltests
branch
2 times, most recently
from
06dec1c
to
964b332
Compare
|
@shindere Sorry for the delay; I pushed a version that works in my machine, hopefully CI will validate.
|
|
Nicolás Ojeda Bär (2018/05/11 07:15 -0700):
@shindere Sorry for the delay;
No problem!
I pushed a version that works in my machine, hopefully CI will
validate.
Hmm, it seems it doesn't :(
I'm sorry but it's not so easy for me to inspect what Travis and
AppVeyor say, so would you (or someoneelse) have a look,please?
For Travis perhaps it's just a matter of adding the
no-change-entry-needed label to the PR?
- I disabled a couple of tests in `mltest.ml` that caused problems but
- that were "repeated" anyway
OK if you feel it's okay it's fine withme.@dra27 if you have an opinion
please do not hesitate to let us know.
- The `exec_tests.ml` test is only run in `native` to avoid the
problem of the missing `unix` DLL. In that test the test executable
is launched from itself using `Unix.exec*` functions. In bytecode,
the `unix` DLL is not found. I did not want to take up more time
porting this test since it is one of the few remaining ones. The
`bytecode` version can always be re-enabled later.
In theory I would agree but after having looked at the code, what I
actually find surprising is not that it's failing in bytecofde but
rather that it's working in native code.
If the program is calling itself, then it is up to the program to make
sure the necessary variables are passed on so it should I think add
CAML_LD_LIBRARY_PATH to the environment of its son.
As an example, see how testsuite/tests/lib-unix/common/redirections.ml
passes OCAMLRUNPARAM in the environment of its son when it calls the
reflector program.
Do you think this can fix the problem?
Regarding exec_tests.precheck: there is a comment there saying
"exec_tests.ml disabled because it fails non-deterministically (at least
under CI)"and you re-enabled the test. Do you think it will work better
now?
|
|
By the way, you may want to pass OCAMLRUNPARAM, too, because if the test
is runwith th debug runtime and OCAMLRUNPARAM is not passed then the
child program will produce output that can mess up the test results.
|
|
Hello, Nicolas,
Just tomake sure: was my review precise enough to be helpful?
Best wishes,
Sébastien.
|
Yeah, sorry for the delay, I've been super busy - I will try to get all this sorted out as soon as possible. |
|
Nicolás Ojeda Bär (2018/05/17 13:36 -0700):
> Just tomake sure: was my review precise enough to be helpful?
Yeah, sorry for the delay, I've been super busy - I will try to get
all this sorted out as soon as possible.
Cool, thanks. I'm really grateful for your help. I am perfectly aware
thatthis kind of job is not exactly themostfun to do on earth. Thanks
gain!
|
|
@nojb: You may take inspiration from |
Thanks, I'll keep it in mind! |
|
@shindere I simplified the test (in particular, no C stubs are needed anymore) and rewrote it as a |
|
@nojb Many thanks!
I think Travis failing is not a big deal.
Tomorrow I'll run this through precheck to double check and if that
passes, too, I'll merge.
Could you please make sure the branch is rebased on latest trunk and the
commit history is clean?
|
nojb
force-pushed
the
win_unicode_ocamltests
branch
from
4af7e5d
to
6ab67ad
Compare
Done. |
|
Thanks. Running precheck.
|
|
Both Inria's precheck and AppVeyor were happy and the code looks great
to me so I went ahead and approved / merged this PR. I did check that the
win-unicode tests are really run on Inria's CI and not just skipped.
Many thanks for your hard work on this, @nojb.
The only point I didn't check is that we do not lose anything in terms
of code coverage compared to the way the tests were done before. Can you
confirm this, @nojb?
|
Well, I removed the "exec" tests which were difficult to get right, but I think they are well exercised by the AppVeyor build which invokes many commands with Unicode arguments. |
|
Nicolás Ojeda Bär (2018/05/22 00:37 -0700):
> The only point I didn't check is that we do not lose anything in terms
of code coverage compared to the way the tests were done before.
Well, I removed the "exec" tests which were difficult to get right,
but I think they are well exercised by the AppVeyor build which
invokes many commands with Unicode arguments.
Okay, I trust you on that, not feeling qualified enough to make a
decision by myself. I guess if somebody wants these test back they can
always submit another PR.
|
cc @shindere