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

A `-promote` option for ocamltest #1586

Merged
merged 5 commits into from Feb 7, 2018

Conversation

Projects
None yet
6 participants
@gasche
Copy link
Member

gasche commented Jan 28, 2018

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

#1574

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

#1519

I checked manually that this option works as expected both for
expect-style tests and for normal foo.reference tests.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Jan 29, 2018

I don't know ocamltest at all but do I understand correctly that when you pass -promote and there is some diff then:

  • you promote the .corrected file
  • you say you promoted the .corrected file
  • the test still counts as failed

?

Doesn't that seem strange?

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Jan 30, 2018

@trefis yes, you understand correctly. I don't know whether this is
strange, and I think that the fact that this is the simplest thing to
implement suggests that it may be a reasonable behavior. Some extra
arguments:

  • If I changed the return value to claim that the test succeeds, then
    you wouldn't have a way to know from the output what may have been
    promoted. I could change the return value to a new "promoted" value,
    but I'd rather not pollute the (arguably critical) success/failure
    logic with extraneous concerns.

  • In any case, we don't really care about what information is returned
    to the user during a -promote run (it could be silent, or just
    return a list of what was promoted); the workflow is that if you see
    that some test fails, you have the opportunity to re-run them with
    -promote (so at this point you already know the test result), then
    you have to git diff to ensure that you aren't promoting crap, and
    then you have to do another test round to see if things still
    fail. The promotion run in the middle doesn't matter.

This raises two questions (coming with their answers):

  1. Why is it that a test may still fail after I ran it with the
    -promote flag on? The answer is that promote only works for
    diff-these-two-files tests (and, with the current patchset, is not
    called if the diff tool fails with an error instead of returning
    a difference). There are other test modes or other failures modes
    where promotion doesn't really make sense, and I think that's
    something we just have to accept -- it comes with expressiveness in
    writing tests. Over time we can think of enriching -promote logic
    to deal with more kinds of tests.

  2. Why do I need to run the whole test logic again just to promote? In
    theory it could be possible for an external-tool to do the
    promotion work, or for ocamltest to decide what to promote by
    inspecting its own logs/reports instead of re-running the test. In
    practice this is much more complex than with the old dumb
    .reference Makefile harness, so I think re-running the test is
    much more robust (I don't have to change the control-flow of
    the tool).

There is one better output that I can think of: not printing anything
for tests that just work, only printing things for failing tests, and
print "success" (or "promoted") for tests that work after promotion
and "failure" for those that still don't work. It sounds like a lot
more work to implement (and review) that, so I don't plan to do it for
now -- I think it would be a case of "the best is the enemy of the
good", given that the promotion workflow again suggests to just ignore
the output anyway.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Jan 30, 2018

One brief comment just on your last point @gasche - I am always very nervous of inferring success from no output (this is possibly because for many years silent termination was the effect of native code stack overflow on Windows), and I don't think that any mode of ocamltest should do that.

@objmagic

This comment has been minimized.

Copy link
Contributor

objmagic commented Jan 31, 2018

The code looks good to me. If we follow this change, we can get back the same old make promote workflow. However, I prefer not to add the "promote" functionality into ocamltest. Leaving it in Makefile might be a better option.

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Jan 31, 2018

I am not at all convinced that using a Makefile is a better option. For example, the proposed makefile only handles expect test, not .reference files, and it's not completely clear to me how to do that (given that with ocamltest you may or may not have different reference files for different compilers, etc.).

@sliquister, as the other of the "expect-promote" Makefile, would you maybe have an opinion on what approach would be best?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Jan 31, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Jan 31, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Jan 31, 2018

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Jan 31, 2018

@gasche not what you asked, it seems that the way you implemented promotion of tests, one can't promote results of a previous run, you have the choice between running tests, and running+promoting tests. Is that right? That'd be an improvement compared to now, but that might be a bit annoying.

I don't see a reason to use a makefile for promotion. As others have said, all the conventions around file placement are in ocamltest already, it's certainly simpler to go with that.
Of course, one can still create a promote target in testsuite/Makefile and make that call ocamltest -promote, so that promotion is available to people using makefiles.

@gasche gasche force-pushed the gasche:ocamltest-promote branch from 6fc65c3 to 1ecc5a2 Feb 2, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Feb 2, 2018

I implemented a promote target for testsuite/Makefile and added it to the PR.

I must say that the feedback so far is giving me pause: my general approach is that having the feature available is better than not having the feature available, but what I heard so far is mostly (reasonable) questions about the implementation approach, and no sign of support for merging it. Is there someone else than myself that thinks that the PR is a good idea?

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Feb 2, 2018

@gasche - I'm not against it, but I never used make promote previously. If I've changed a test (which, as the blame shows, I do quite a bit!), I expect to spend most of my thought process during git add -p when I'm considering whether the alterations to the files are correct - I find the cp foo.res* foo.ref* to be something I almost subconsciously type before starting that.

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Feb 2, 2018

An exemple wherepromote can be very useful is GPR#1511's diff, when a change to a tool affects 66 different reference files over many different directories. In this particular case the change was to expect itself, so only expect-style tests were affected, but changes to error/warning behaviours could affect tests using different drivers and test tools. The manual-fixing approach doesn't really scale for those cross-cutting changes.

@objmagic

This comment has been minimized.

Copy link
Contributor

objmagic commented Feb 3, 2018

@gasche I am neutral about implementation, and I am strongly in favor of having promote back. Personally I find it very useful and I use it all the time!

@trefis

trefis approved these changes Feb 6, 2018

Copy link
Contributor

trefis left a comment

Seems that no one formally approved, so let me do so: there is no doubt in my mind that the -promote option is necessary, so we should merge it as long as the implementation isn't completely ridiculous (and I believe it is not completely ridiculous).

Concerning my previous comment, I had missed the fact that ocamltest only prints in its log that a file was promoted, not on stdout/stderr. So I now understand why you decided to still count the test as failed.
That situation makes me sad, but it can be discussed independently.

As sliquister already pointed out, I also find slightly sad that one can't promote the result of a previous run but has to rerun the tests to be able to promote.
I have no doubt that this was the easiest way to implement -promote. I do hope it can be changed though.

All that said, this PR is an improvement over the current state of affairs, so I'm in favor of merging now and letting brave souls reworking the implementation at a latter date.

PS: I had a look at the makefile changes, they look OK to me, but I'm no expert.

@gasche gasche force-pushed the gasche:ocamltest-promote branch from 1ecc5a2 to 746dfc5 Feb 7, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Feb 7, 2018

I just rebased the PR against trunk.

@shindere: in the end I did not rename -promote in -p as we briefly discussed, because I'm not sure anymore which is best so I decided to leave it as-is. I still think that it will be perfectly okay to change the interface later if you work on a better way to implement promotion.

gasche added some commits 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

  #1574

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

  #1519
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 7, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

gasche commented Feb 7, 2018

I documented the experimental status in cb2379f.

@gasche gasche merged commit f5a1ec5 into ocaml:trunk Feb 7, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.