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

More command line options for expect tests #1502

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
2 participants
@Octachron
Copy link
Contributor

Octachron commented Nov 30, 2017

This PR (mostly cherry-picked from #1481) updates the expect_test tool in order to handle most of the options of the toplevel; this was needed for testing the error size option in #1481, and @Armael mentioned recently that he might also use this possibility. It also adds a new makefile variable for passing such options to expect_test for makefile-based tests.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 30, 2017

Seems reasonable to me. Do you have ideas of how we may go at reducing the redundancy in functor argument definitions? It looks like some of the "obvious Clflags setters" could be exported on their own (as a sub-signature of the functor input type?) and included in several places.

(I think the PR would be marginally nicer with at least one test making use of this capability, so that we see what it looks like on the user side. Any idea for a simple/short example?)

let _args = Arg.read_arg
let _args0 = Arg.read_arg0

let anonymous s = (*disabled*) ()

This comment has been minimized.

@gasche

gasche Nov 30, 2017

Member

Does (* disabled *) () here have the effect that anonymous parameters are silently ignored? If that was the case, I think it would not be a very good choice.

This comment has been minimized.

@Octachron

Octachron Nov 30, 2017

Author Contributor

No, the comment is unclear; the anonymous arguments are handled by the main function in expect_test, this anonymous function is only here to fulfill the functor argument signature.

This comment has been minimized.

@gasche

gasche Nov 30, 2017

Member

If I understand the code correctly, passing the command-line arguments -- foo would still call the anonymous functor argument on foo. (If there was no way to have this functor argument ever called at run time, this would be an API bug and we would consider removing it.)

This comment has been minimized.

@Octachron

Octachron Nov 30, 2017

Author Contributor

I stand corrected; - -a.ml will indeed call anonymous "-a.ml". Fixed.

@Octachron Octachron force-pushed the Octachron:expect_options branch from eaf734d to 389af30 Nov 30, 2017

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Nov 30, 2017

Factoring a set of core arguments might work as a way to reduce the redundancy between the definition of arguments in {ocamlc,ocamlopt,ocaml,ocamlnat,expect_test}. In term of tests, this PR is enough for converting the short-path tests in typing-short-path to expect tests for instance. Or for a toy test, I could add an expect test for the non applicative functor mode -no-app-funct which is not tested in the test suite currently.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 30, 2017

It looks like you need a Changes entry.

In term of tests, this PR is enough for converting the -short-path tests in typing-short-path to expect tests for instance. Or for a toy test, I could add an expect test for the non applicative functor mode -no-app-funct which is not tested in the test suite currently.

Please do what you think is best for this PR -- including the option of doing nothing.

@gasche

gasche approved these changes Nov 30, 2017

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 30, 2017

I approved the PR as I reviewed it, believe that it is (now) correct and useful, and is good to merge as-is. There is the question of having a small test (see above), and also the question of whether or not to factorize is left open (please confirm that you would rather merge as-is, or indicate that you would like to try to factorize a bit).

@Octachron Octachron force-pushed the Octachron:expect_options branch from 389af30 to a035ee9 Nov 30, 2017

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Nov 30, 2017

I have added a changelog entry.

I would rather have a separated PR for migrating the short-paths test to expect tests; and a short test for -no-app-funct will work better with ocamltest. Similarly, refactoring the argument code feels quite contingent to this PR, thus I think that it is fine to merge as it is.

@gasche gasche merged commit 64eee99 into ocaml:trunk Dec 1, 2017

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.
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.