Adds a new 'inline-tests' setting in env stanza #2313

mlasson · 2019-06-25T09:59:27Z

This PR propose to add new field "inline-tests" than can be used in the env stanza.

For instance,

  (env
     (profile1 (inline-tests enabled))
     (profile2 (inline-tests disabled))
     (profile3 (inline-tests ignored))
  )

The only possible values are enabled/disabled/ignored evrything else is a sytanx error.
Then we provide a new template variable %{inline-tests} that expands to "enabled"/"disabled"/"ignored" according to the current profile. The default value is "enabled" except if the profile is "release" in that case it is "disabled".

TODO

: Add a paragraph in the manual to document the feature,
: Update CHANGES,
: Add a test.
: Rename "inline-tests" into "inline_tests" for consistency.

aalekseyev · 2019-06-25T13:56:20Z

This seems to suggest that inline tests are somehow a special case of instrumentation. I don't understand that.

In the thread you linked @diml proposed to use a variable %{inline-tests} for inline tests. That seems good to me. The variant [`ignored|`disabled|`enabled] is in fact designed specifically for inline tests because for those there's a well-defined meaning of "disabled" and "ignored".

More generally, I feel that we should only introduce special configuration mechanisms for things that have special support from Dune. Inline tests do, but instrumentation tools don't.

So I think for instrumentation tools we should use a general mechanism of passing untyped information to preprocessors. Maybe env_vars can work?

mlasson · 2019-06-25T14:19:46Z

The general problem that this aims to solve is to provide a mechanism for passing a variable PPX libraries that is enabled/disabled according to the profile and "inline-tests" could be an instance of that.

I can see that it may look confusing and that there's not a strong demand for solving the broader problem. So, if you guys think we don't want to go that way, I don't mind at all renaming "instrumentation" into "inline-tests" and forget all about the future extension. It was just a proposal.

Also, currently the string values can contain variables, I could also remove this feature and force the string to be one of the value in "enabled"/"disabled"/"ignored".

aalekseyev · 2019-06-25T14:40:44Z

Oh, I see. This does seem like a tricky problem, as in: it's tricky to know what we even want.

If you want exactly a boolean that defaults to true in release profile with no extra meaning attached, then it could be called "in_release_profile" or some such.

That's almost equivalent to matching on profile name, but it avoids the main disadvantages of that: it lets people to override the property locally, or set it to true in multiple profiles if necessary.

mlasson · 2019-06-25T14:55:52Z

Ok, I think we should stick to the simplest/original plan.
I'll do the renaming and also the restriction to the three possible values.
Frankly, I don't mind the matching the profile and/or using env variable to control "instrumentation" and inline tests have a real user base, it deserves its own setting.

ghost · 2019-07-01T09:50:12Z

src/dune_env.ml

+  let inline_tests_field =
+    field_o
+    "inline-tests"
+      (Syntax.since Stanza.syntax (1, 10) >>>


Suggested change

(Syntax.since Stanza.syntax (1, 10) >>>

(Syntax.since Stanza.syntax (1, 11) >>>

ghost · 2019-07-01T09:58:47Z

Sticking to the simplest proposal seems best for now indeed

mlasson · 2019-07-19T17:23:12Z

( I inadvertently requested a review from the whole world by messing with git :/, sorry about spamming you guys )

mlasson · 2019-07-19T17:27:05Z

doc/dune-files.rst

+- ``(inline-tests <state>)`` where state is either ``enabled``, ``disabled`` or
+  ``ignored``. This field controls the value of the variable ``%{inline-tests}``
+  that is read by the inline test framework. The default value is ``disabled``
+  for the ``release`` profile and ``enabled`` otherwise.


TODO:

: Add a since 1.11 ...

mlasson · 2019-07-19T17:28:43Z

CHANGES.md

@@ -8,6 +8,9 @@
 - Do not put the `<package>.install` files in the source tree unless `-p` or
  `--promote-install-files` is passed on the command line (#2329, @diml)

+- Add a new `inline-tests` field in the env stanza to control inline-tests


TODO:

: should move this in the 1.11 section ...

src/env_node.ml

rgrinberg · 2019-07-22T09:12:20Z

Okay, this looks good. Just needs a few tests and this should be ready for 1.11

Signed-off-by: Marc Lasson <[email protected]>

rgrinberg · 2019-07-22T09:48:21Z

test/blackbox-tests/test-cases/inline_tests/run.t

  [1]

+  $ env -u OCAMLRUNPARAM dune runtest simple --profile release


Could you comment this test? It's a bit confusing to understand when it has no output.

Signed-off-by: Marc Lasson <[email protected]>

@TheLortex

CHANGES: - Don't select all local implementations in `dune utop`. Instead, let the default implementation selection do its job. (ocaml/dune#2327, fixes ocaml/dune#2323, @TheLortex, review by @rgrinberg) - Check that selected implementations (either by variants or default implementations) are indeed implementations. (ocaml/dune#2328, @TheLortex, review by @rgrinberg) - Don't reserve the `Ppx` toplevel module name for ppx rewriters (ocaml/dune#2242, @diml) - Redesign of the library variant feature according to the ocaml/dune#2134 proposal. The set of variants is now computed when the virtual library is installed. Introducing a new `external_variant` stanza. (ocaml/dune#2169, fixes ocaml/dune#2134, @TheLortex, review by @diml) - Add proper line directives when copying `.cc` and `.cxx` sources (ocaml/dune#2275, @rgrinberg) - Fix error message for missing C++ sources. The `.cc` extension was always ignored before. (ocaml/dune#2275, @rgrinberg) - Add `$ dune init project` subcommand to create project boilerplate according to a common template. (ocaml/dune#2185, fixes ocaml/dune#159, @shonfeder) - Allow to run inline tests in javascript with nodejs (ocaml/dune#2266, @hhugo) - Build `ppx.exe` as compiling host binary. (ocaml/dune#2286, fixes ocaml/dune#2252, @toots, review by @rgrinberg and @diml) - Add a `cinaps` extension and stanza for better integration with the [cinaps tool](https://github.com/janestreet/cinaps) tool (ocaml/dune#2269, @diml) - Allow to embed build info in executables such as version and list and version of statically linked libraries (ocaml/dune#2224, @diml) - Set version in `META` and `dune-package` files to the one read from the vcs when no other version is available (ocaml/dune#2224, @diml) - Add a variable `%{target}` to be used in situations where the context requires at most one word, so `%{targets}` can be confusing; stdout redirections and "-o" arguments of various tools are the main use case; also, introduce a separate field `target` that must be used instead of `targets` in those situations. (ocaml/dune#2341, @aalekseyev) - Fix dependency graph of wrapped_compat modules. Previously, the dependency on the user written entry module was omitted. (ocaml/dune#2305, @rgrinberg) - Allow to promote executables built with an `executable` stanza (ocaml/dune#2379, @diml) - When instantiating an implementation with a variant, make sure it matches virtual library's list of known implementations. (ocaml/dune#2361, fixes ocaml/dune#2322, @TheLortex, review by @rgrinberg) - Add a variable `%{ignoring_promoted_rules}` that is `true` when `--ingore-promoted-rules` is passed on the command line and false otherwise (ocaml/dune#2382, @diml) - Fix a bug in `future_syntax` where the characters `@` and `&` were not distinguished in the names of binding operators (`let@` was the same as `let&`) (ocaml/dune#2376, @aalekseyev, @diml) - Workspaces with non unique project names are now supported. (ocaml/dune#2377, fix ocaml/dune#2325, @rgrinberg) - Improve opam generation to include the `dune` dependncies with the minimum constraint set based on the dune language version specified in the `dune-project` file. (2383, @avsm) - The order of fields in the generated opam file now follows order preferred in opam-lib. (@avsm, ocaml/dune#2380) - Fix coloring of error messages from the compiler (@diml, ocaml/dune#2384) - Add warning `66` to default set of warnings starting for dune projects with language verison >= `1.11` (@rgrinberg, @diml, fixes ocaml/dune#2299) - Add (dialect ...) stanza (@nojb, ocaml/dune#2404) - Add a `--context` argument to `dune install/uninstall` (@diml, ocaml/dune#2412) - Do not warn about merlin files pre 1.9. This warning can only be disabled in 1.9 (ocaml/dune#2421, fixes ocaml/dune#2399, @emillon) - Add a new `inline_tests` field in the env stanza to control inline_tests framework with a variable (ocaml/dune#2313, @mlasson, original idea by @diml, review by @rgrinberg). - New binary kind `js` for executables in order to explicitly enable Javascript targets, and a switch `(explicit_js_mode)` to require this mode in order to declare JS targets corresponding to executables. (ocaml/dune#1941, @nojb)

mlasson changed the title ~~Adds a new 'instrumentation' setting in env stanza~~ [RFC] Adds a new 'inline_tests/instrumentation/in_release_profile' setting in env stanza Jun 25, 2019

mlasson force-pushed the mlasson-instrumentation-flags branch from 2370365 to 2dcf9a4 Compare June 25, 2019 15:46

ghost suggested changes Jul 1, 2019

View reviewed changes

mlasson changed the title ~~[RFC] Adds a new 'inline_tests/instrumentation/in_release_profile' setting in env stanza~~ Adds a new 'inline_tests' setting in env stanza Jul 19, 2019

mlasson changed the title ~~Adds a new 'inline_tests' setting in env stanza~~ Adds a new 'inline-tests' setting in env stanza Jul 19, 2019

mlasson force-pushed the mlasson-instrumentation-flags branch from 2dcf9a4 to 6b731d1 Compare July 19, 2019 17:14

mlasson requested review from aalekseyev, ejgallego, emillon, fpottier, hhugo and rgrinberg as code owners July 19, 2019 17:14

mlasson force-pushed the mlasson-instrumentation-flags branch from 6b731d1 to 865fc73 Compare July 19, 2019 17:20

mlasson commented Jul 19, 2019

View reviewed changes

rgrinberg requested changes Jul 22, 2019

View reviewed changes

src/env_node.ml Outdated Show resolved Hide resolved

mlasson force-pushed the mlasson-instrumentation-flags branch from b8faaa3 to fc2b677 Compare July 22, 2019 09:04

mlasson added 4 commits July 22, 2019 11:13

Adds a new 'instrumentation' setting in env stanza

c05dbdf

Signed-off-by: Marc Lasson <[email protected]>

Default to the parent node

c9c0e7c

Signed-off-by: Marc Lasson <[email protected]>

Rename iniline-tests -> inline_tests

ab71f06

Signed-off-by: Marc Lasson <[email protected]>

inline_tests field in env since 1.11

8010f81

Signed-off-by: Marc Lasson <[email protected]>

mlasson force-pushed the mlasson-instrumentation-flags branch from fc2b677 to 8010f81 Compare July 22, 2019 09:14

Typo

87f5816

Signed-off-by: Marc Lasson <[email protected]>

rgrinberg reviewed Jul 22, 2019

View reviewed changes

mlasson added 2 commits July 22, 2019 11:58

Adds a few tests

3dbe5f1

Signed-off-by: Marc Lasson <[email protected]>

Adds a comment to explain why no output

05f9a9e

Signed-off-by: Marc Lasson <[email protected]>

rgrinberg approved these changes Jul 22, 2019

View reviewed changes

mlasson merged commit c85fd6d into ocaml:master Jul 22, 2019

mlasson deleted the mlasson-instrumentation-flags branch July 22, 2019 15:33

rgrinberg mentioned this pull request Jul 22, 2019

[new release] dune-build-info and dune (1.11.0) ocaml/opam-repository#14567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a new 'inline-tests' setting in env stanza #2313

Adds a new 'inline-tests' setting in env stanza #2313

mlasson commented Jun 25, 2019 •

edited

Loading

aalekseyev commented Jun 25, 2019 •

edited

Loading

mlasson commented Jun 25, 2019

aalekseyev commented Jun 25, 2019

mlasson commented Jun 25, 2019

ghost Jul 1, 2019

ghost commented Jul 1, 2019

mlasson commented Jul 19, 2019

mlasson Jul 19, 2019 •

edited

Loading

mlasson Jul 19, 2019 •

edited

Loading

rgrinberg commented Jul 22, 2019

rgrinberg Jul 22, 2019

	(Syntax.since Stanza.syntax (1, 10) >>>
	(Syntax.since Stanza.syntax (1, 11) >>>

		[1]

		$ env -u OCAMLRUNPARAM dune runtest simple --profile release

Adds a new 'inline-tests' setting in env stanza #2313

Adds a new 'inline-tests' setting in env stanza #2313

Conversation

mlasson commented Jun 25, 2019 • edited Loading

TODO

aalekseyev commented Jun 25, 2019 • edited Loading

mlasson commented Jun 25, 2019

aalekseyev commented Jun 25, 2019

mlasson commented Jun 25, 2019

ghost Jul 1, 2019

Choose a reason for hiding this comment

ghost commented Jul 1, 2019

mlasson commented Jul 19, 2019

mlasson Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

mlasson Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

rgrinberg commented Jul 22, 2019

rgrinberg Jul 22, 2019

Choose a reason for hiding this comment

mlasson commented Jun 25, 2019 •

edited

Loading

aalekseyev commented Jun 25, 2019 •

edited

Loading

mlasson Jul 19, 2019 •

edited

Loading

mlasson Jul 19, 2019 •

edited

Loading