Skip to content
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

Migrate expect tests to ocamltest #1519

Merged
merged 13 commits into from Dec 12, 2017

Conversation

Projects
None yet
3 participants
@shindere
Copy link
Contributor

shindere commented Dec 8, 2017

Cc @diml

Most of the tests could be migrated in a straightforward way.

One notable exception is the pr6939 test in typing-misc.

For this one, two actions have been added to ocamltest to figure out
whether flat float arrays are enabled or not.

In the long run, I am wondering whether expect couldn't be integrated
to ocamltest.

At the moment, I suggest to move expect to the ocamltest directory so
that it gets compiled alongside.

If there is an agreement on this, I'd be happy to add a commit doing that
to this PR.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 8, 2017

Cc @diml

@shindere shindere force-pushed the shindere:migrate-expect-tests branch from e58260e to adfa328 Dec 8, 2017

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 8, 2017

FWIW, this PR passes Inria's precheck in addition to AppVeyor and Travis.

@@ -24,6 +24,10 @@ val dumpenv : Actions.t
val unix : Actions.t
val windows : Actions.t

val setup_build_env : Actions.t

val setup_simple_build_env : Actions.t

This comment has been minimized.

@gasche

gasche Dec 11, 2017

Member

What is setup_simple_build_env doing compared to setup_build_env?

[
Builtin_variables.reference, input_file;
Builtin_variables.output, output_file
] env2 in

This comment has been minimized.

@gasche

gasche Dec 11, 2017

Member

An intermediary file input_file ^ ".corrected" is generated by running the command twice. Is this intermediary file simply ignored, or will it get automatically cleaned by ocamltest?

flags env;
repo_root;
principal_flag;
input_file

This comment has been minimized.

@gasche

gasche Dec 11, 2017

Member

The original invocation command

TERM=dumb $(EXPECT_TEST) $(EXPECT_FLAGS) -repo-root $(OTOPDIR) $$file

also starts with a TERM=dumb setting to control the behavior of toplevel location printing (I suppose). Is it reproduced here, for example by being globally set by ocamltest, or by Actions_helpers.run_cmd?

Builtin_variables.output, output_file
] env2 in
Pass output_env
| Skip _ | Fail _ -> second_run

This comment has been minimized.

@gasche

gasche Dec 11, 2017

Member

This is minor, but I find the code more readable if the short cases are first (Skip _ | Fail _ before Pass env). Otherwise when I read the short cases, I don't remember well what was being matched.

Actions_helpers.run_script log newenv
let ocamlsrcdir = ocamlsrcdir () in
let input_file = Actions_helpers.testfile env in
run_expect_twice ocamlsrcdir input_file log env

This comment has been minimized.

@gasche

gasche Dec 11, 2017

Member

When are the result and reference file actually compared? I don't see a call to Actions_helpers.check_output, which I thought was used for this.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 12, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 12, 2017

Yes, thanks for the clarification on the setup{,_simple}_build_env functions.

Re. input_file ^ ".corrected"

No the intermediate file is generated by the first run of the command.
It's used as input for the second run of the command.

I understand that, but I wonder what becomes of this intermediate file after the second run. Is it removed / cleaned up ? In the original Makefile, mv foo.corrected.corrected foo.corrected is performed after the second run, which guarantees that, by the time the script terminates, only two files are left on the disk, the original input file and the output file (of the second run).

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 12, 2017

@gasche

gasche approved these changes Dec 12, 2017

Copy link
Member

gasche left a comment

Thanks for the clarifications. Good to merge.

@gasche gasche merged commit b0a59c3 into ocaml:trunk Dec 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 12, 2017

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 12, 2017

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 12, 2017

@gasche why did you add a Changes entry for this PR?

Do you intent to do that for all the test migration PRs?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 12, 2017

I thought that there was enough non-trivial work in this PR that you deserved to be credited for the work (and I for the review!). I guessed that you had opted out of the the Changes-entry-writing business because you might not be so comfortable with it, so instead of pinging you to write one before I merge I decided to just write it.

We can consolidate this entry for other test migrations, and/or only mention the ones that require significant amount of adaptation work.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 12, 2017

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 13, 2017

I just rebased past this.
Can ocamltest put TERM=dumb in the environment of ocaml-expect, otherwise the tests break because colors? Also is there any way to see the diff of the failure now, and to promote the corrected file?

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 13, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

Promoting matters in practice when operating on a large number of tests whose output has to change. For example when working on #1511, I had to update the reference files of all the expect tests in the testsuite, and this was done with a for f in tests/*; do cd $f; make promote; done.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 13, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

Right now you can compute a diff between the expected output and the output; it may be possible to add a --promote mode to ocamltest where the output is copied into the expected output instead of being compared.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 13, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

I think that the file(s) that should be promoted is precisely the one(s) that cause(s) the test to fail. So I'd wait until ocamltest has decided which reference file to use, and promote this one.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 13, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

If the test that I am running fails because the output (however it is computed) does not correspond to foo.compilers.reference (however this reference file is determined), then foo.compilers.reference should be modified. If it fails because the output does not correspond to bar.ocamlc.byte.reference, then that file should be modified. Doing anything less simple than that would not be a good idea.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 13, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 13, 2017

Then the user will notice that there is an issue (because the tests will still fail after promotion) and will have to think harder about the problem, which is what we want to happen: if all compiler outputs used to be the same and now they are not, something interesting occurred and we need human supervision.

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 13, 2017

#1491 is what I was rebasing.

Normally the log file of the tests should include a diff when the result differs from the reference.

I don't get it. It sounds like you're saying the way to see the output of a failing test, beyond whether it failed or not, is something like:

$ ocamltest *.ml
$ grep '' _ocamltest/*/*.log

(well actually that would show both failing and passing tests, so this command is actually 90% noise)

So in practice, it looks like the way to go for now is:

$ ocamltest *.ml
$ for v in _ocamltest/*; do diff $v/*.ml $v/*.corrected.corrected; done

which is not noisy and allows me to use the diff program I want.

and to promote the corrected file?
No, that has not been implemented. But promoting is just a matter of moving / renaming a file, right?

Yes, but given that the .corrected files are hidden in the _ocamltest directory, it's annoying to actually find which command to type.

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 14, 2017

Also: ocamltest is leaking all these ocamltestXXXXXXdiff files, in filecompare.ml.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 14, 2017

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Dec 14, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 14, 2017

make promote does more than showing the diff, it also does exactly what is needed to update the reference files to have the test pass, so then it suffices to git commit when you did a change that affects (in a benign way) many tests.

Just not now, if nobody minds. Theplan is really to migrate as many tests as possible, as quickly as possible, so that we can get rid of the legacy framework.

It is a bit uncomfortable to break the workflow of people writing tests today when we migrate.

(Another workflow that is not available anymore in the new system is to perform the tests in parallel (I realized while trying to reproduce the color issue), but I think that this is easy to add and I'll try to send a PR soon.)

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 15, 2017

Yeah the workflow is kind of broken right now.
Before:

  • cd testsuite/bla/bla/bla
  • run make DIFF="diff -u" deps default, to see the test result
  • run make promote, accept the changes

Now:

  • make world etc
  • cd testsuite/bla/bla/bla
  • run ../../../ocamltest/ocamltest *.ml; for v in _ocamltest/*; do diff -u $v/*.ml $v/*.corrected.corrected; done to see the test result
  • run some other script that finds all the .corrected.corrected and renames to their source files, to accept the changes

I can live with the current state of things, because now I know enough to script around it, but I'm guessing other people will run into this (or run all the tests I suppose, but you can't iterate with that, it's horribly slow).

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Dec 15, 2017

In case it's useful to others, the script below should allow one to work on a single expect-test (doesn't help with @gasche's problem of promoting all tests).

The workflow becomes (even from a clean repo):

  • cd testsuite/bla/bla/bla
  • ocamltest, to see the test result
  • ocamltest promote, to accept the change

No need for configure, or make world, or ../../../ocamltest/ocamltest, etc

$ cat ocamltest
#!/bin/bash

# Help:
# ocamltest: runs the expect tests in the current directory, after building
#            everything that's necessary
# ocamltest promote: promote the output of last run of tests to be the expected output

set -e -u -o pipefail

if [ $# -ge 1 ]; then
    case "$1" in
	promote)
	    shopt -s nullglob
	    for v in _ocamltest/*/*.corrected.corrected; do
		mv $v $(basename $v .corrected.corrected)
	    done
	    exit 0
	    ;;
	*) echo >&2 "unknown command $1"
	   exit 1
	   ;;
    esac
fi

root=$(git rev-parse --show-toplevel)
start_dir=$PWD

cd "$root"
if ! [ -f utils/config.ml ]; then
  ./configure -prefix "$PWD"/installed
fi
make coldstart ocaml ocamlc runtime
make -C testsuite/tools expect_test
make -C lex
make -C ocamltest

cd "$start_dir"
rm -rf _ocamltest
"$root"/ocamltest/ocamltest *.ml
function diff {
    if ! cmp -s "$@"; then
	if which patdiff &> /dev/null; then
	    patdiff "$@" && patdiff -keep-whitespace "$@" && command diff -u "$@"
	else
	    command diff -u "$@"
	fi
    fi
}
for v in _ocamltest/*; do ${DIFF-diff} $v/*.ml $v/*.corrected.corrected; done

@sliquister sliquister referenced this pull request Dec 18, 2017

Merged

ocamltest #681

gasche added a commit to gasche/ocaml that referenced this pull request Jan 28, 2018

ocamltest: implement a -promote option
When -promote is set, running the test automatically overwrites each
reference output file with the actual output of the test. This option,
which mirrors the "make promote" target of old-style testsuite tests,
is very useful when a minor change in compiler/toplevel output affects
a lot of reference files in innocuous ways.

This is the only option of ocamltest that overwrites file in the
source directory, so it should be used with care -- under strict
version-control supervision.

The 'make promote' target is not yet supported for new-style testsuite
tests, as this feature depends on the not-yet-merged GPR#1574

  ocaml#1574

For a previous discussions of this feature, see GPR#1519

  ocaml#1519

gasche added a commit to gasche/ocaml that referenced this pull request Feb 2, 2018

ocamltest: implement a -promote option
When -promote is set, running the test automatically overwrites each
reference output file with the actual output of the test. This option,
which mirrors the "make promote" target of old-style testsuite tests,
is very useful when a minor change in compiler/toplevel output affects
a lot of reference files in innocuous ways.

This is the only option of ocamltest that overwrites file in the
source directory, so it should be used with care -- under strict
version-control supervision.

The 'make promote' target is not yet supported for new-style testsuite
tests, as this feature depends on the not-yet-merged GPR#1574

  ocaml#1574

For a previous discussions of this feature, see GPR#1519

  ocaml#1519

gasche added a commit to gasche/ocaml that referenced this pull request Feb 7, 2018

ocamltest: implement a -promote option
When -promote is set, running the test automatically overwrites each
reference output file with the actual output of the test. This option,
which mirrors the "make promote" target of old-style testsuite tests,
is very useful when a minor change in compiler/toplevel output affects
a lot of reference files in innocuous ways.

This is the only option of ocamltest that overwrites file in the
source directory, so it should be used with care -- under strict
version-control supervision.

The 'make promote' target is not yet supported for new-style testsuite
tests, as this feature depends on the not-yet-merged GPR#1574

  ocaml#1574

For a previous discussions of this feature, see GPR#1519

  ocaml#1519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.