feature(cram): allow overriding default alias by rgrinberg · Pull Request #8887 · ocaml/dune

rgrinberg · 2023-10-09T01:04:03Z

Introduce a [(default_aliases ..)] that allows to set the default alias
for a cram test.

Unlike, the existing [alias] field, we're only allowed to set the
default once. If the default isn't set, it's just [runtest].

Signed-off-by: Rudi Grinberg me@rgrinberg.com

Alizter · 2023-10-09T10:47:51Z

It would have probably been better to have an (aliases :standard) field IMO. But I don't know a good way to get people to transition to using it if we ever switch (aliases foo) -> (aliases :standard foo).

rgrinberg · 2023-10-09T17:51:04Z

@gridbugs do you want to review this one?

gridbugs · 2023-10-10T01:24:43Z

test/blackbox-tests/test-cases/cram/default-aliases.t

+  Error: Files _build/default/foo.t and _build/default/foo.t.corrected differ.
+  [1]
+
+Now we try setting the defualt aliases twice. This should be impossible:


Suggested change

Now we try setting the defualt aliases twice. This should be impossible:

Now we try setting the default aliases twice. This should be impossible:

gridbugs · 2023-10-10T01:36:10Z

Can you please update the docs with the new field. Also it's not clear to me what it means to change the default alias for a test. It looks like you always need to specify the name of the alias in order to run the test so in what sense is it a default? From looking at the example it seems like the behaviour of this feature is that tests can be grouped together by specifying the same (default_aliases <name>) an then you can build that alias to run all the associated tests. Is that right? Maybe the field should be named group_alias and a separate flag should allow a test to be excluded from @runtest.

rgrinberg · 2023-10-10T03:26:15Z

It looks like you always need to specify the name of the alias in order to run the test so in what sense is it a default?

By default, the alias is set to runtest. This feature allows to override this deafult.

From looking at the example it seems like the behaviour of this feature is that tests can be grouped together by specifying the same (default_aliases ) an then you can build that alias to run all the associated tests. Is that right? Maybe the field should be named group_alias

This property applies to all fields in the (cram ..) stanza. The (applies_to ..) field determines to which fields the stanza applies.

and a separate flag should allow a test to be excluded from @runtest.

Yeah, that could work. We could add a flag like (default_alias <true|false>). It would be very similar to what is done here already, so I'm not sure if it's an improvement.

gridbugs · 2023-10-10T04:26:47Z

By default, the alias is set to runtest. This feature allows to override this deafult.

Do you mean all tests defined in the same directory as the dune file will be associated with the specified alias rather than with @runtest?

rgrinberg · 2023-10-10T04:28:11Z

Yes. Although instead of a single alias, it's a list. With a list, we can also set the default to be the empty list and therefore remove all default aliases.

gridbugs · 2023-10-10T04:32:09Z

Ok that makes sense. I find the naming a bit confusing because there's now a notion of a "default default" alias (@runtest). Like there are two levels of defaults - the default aliases associated with tests and the choice of default alias to use if none is specified.

rgrinberg · 2023-10-10T04:36:02Z

Wait, there's only one level of defaults that I can see. To recap, there's two fields:

The existing (alias ..) field that can only be used to add a new alias to a set of tests. This field isn't able to remove any aliases which is why it cannot be used to solve the problem of removing the default runtest alias.
The new (default_aliases ..) field that can be used to set the default alias instead of runtest. Only this one is a real default.

gridbugs · 2023-10-10T04:39:56Z

If default_aliases is not specified it effectively takes the value runtest, no? That would make runtest the default value for default_aliases.

rgrinberg · 2023-10-10T04:41:46Z

Okay, I see what you mean. What if the name was base_aliases instead?

I suppose your idea of just having a flag (default_alias <true|false>) is also ok.

Any preferences?

gridbugs · 2023-10-10T04:47:56Z

I like the name base_aliases. Any reason to not do both though? I see this change as two distinct features that happen to share an implementation:

allowing a test to be excluded from the set of tests run by dune runtest
allowing a group of related tests to be grouped together under a new alias (this could be particularly useful with pkg - we could add an alias that just runs the pkg tests without removing those tests from @runtest).

So what if we made it so regardless of the value of base_aliases, the tests remain in @runtest unless explicitly excluded with a flag. And rather than calling the flag (default_alias <true|false>) I think it would be clearer to have a (exclude_from_runtest) flag.

rgrinberg · 2023-10-10T04:50:48Z

allowing a group of related tests to be grouped together under a new alias (this could be particularly useful with pkg - we could add an alias that just runs the pkg tests without removing those tests from @runtest).

If you need this, we can already have this:

$ cat test/blackbox-tests/test-cases/pkg/dune
(cram
 (applies_to :whole_subtree)
 (alias pkg))

The only thing that's missing is excluding things from runtest by default.

gridbugs · 2023-10-10T04:51:58Z

It's also not possible to include a test in multiple custom aliases (ie. aliases besides @runtest), right?

rgrinberg · 2023-10-10T04:54:23Z

Nope, it's possible. You just need multiple cram stanzas.

$ cat a/dune
(cram (alias foo))
(cram (alias bar))

Will make all the tests in the directory a/ buildable under 3 aliases:

runtest
foo
bar

gridbugs · 2023-10-10T04:57:02Z

Ah cool. In that case I'd prefer we add a flag that excludes the tests in a directory from @runtest. I would find that UI clearer than setting an empty list of base aliases, since it's not immediately obvious that the reason a test is included in @runtest is because @runtest is the default base alias for all cram tests.

rgrinberg · 2023-10-10T05:10:21Z

Okay, I'll call the flag (runtest_alias <enable|disable>). It will also not be possible to set it more than once per test.

rgrinberg · 2023-10-12T22:00:41Z

@gridbugs updated. If this is OK, I will update the docs.

gridbugs

Looks good to me

doc/changes/8887.md

rgrinberg · 2023-10-13T04:00:44Z

docs are in

gridbugs · 2023-10-13T04:08:46Z

doc/tests.rst

 - ``(package <package-name>)`` - attach the tests selected by this stanza to the
  specified package
+- ``(runtest_alias <true|false>)`` - when set to ``false``, do not add the
+  tests to the ``runtest`` alias by default.


I don't get why "by default" is there. Can you add a note that if this field is omitted then the tests will be included in the @runtest alias. Also what's the use case for (runtest_alias true)? Is there ever a time when the field should be explicitly set to true?

I don't get why "by default" is there. Can you add a note that if this field is omitted then the tests will be included in the @runtest alias

I clarified it.

Also what's the use case for (runtest_alias true)? Is there ever a time when the field should be explicitly set to true?

Mostly to allow things like (runtest_alias %{lib-available:foo}) or blang expressions if there is ever a need.

Experience tells me that flag-like fields (e.g. no_runtest_alias) end up being annoying to use. E.g. suppose that we change the default to not add the runtest alias to cram by default in a later version of the dune language, how would the user be able to switch it back on?

Makes sense. Thanks for clarifying.

Introduce a [(runtest_alias ..)] that allows to enable/disable the runtest alias for cram tests. Unlike, the existing [alias] field, we're only allowed to set the this value once. Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg · 2023-10-13T04:28:04Z

@Alizter feel free to use this to move melange/coq to separate CI jobs.

@EmileTrotignon

CHANGES: - Introduce `$ dune ocaml doc` to open and browse documentation. (ocaml/dune#7262, fixes ocaml/dune#6831, @EmileTrotignon) - `dune cache trim` now accepts binary byte units: `KiB`, `MiB`, etc. (ocaml/dune#8618, @Alizter) - No longer force colors for OCaml 4.03 and 4.04 (ocaml/dune#8778, @rgrinberg) - Introduce new experimental odoc rules (ocaml/dune#8803, @jonjudlam) - Introduce the `runtest_alias` field to the `cram` stanza. This allows removing default `runtest` alias from tests. (@rgrinberg, ocaml/dune#8887) - Do not ignore libraries named `bigarray` when they are defined in conjunction with OCaml 5.0 (ocaml/dune#8902, fixes ocaml/dune#8901, @rgrinberg) - Dependencies in the copying sandbox are now writeable (ocaml/dune#8920, @rgrinberg) - Absent packages shouldn't prevent all rules from being loaded (ocaml/dune#8948, fixes ocaml/dune#8630, @rgrinberg) - Correctly determine the stanza of menhir modules when `(include_subdirs qualified)` is enabled (@rgrinberg, ocaml/dune#8949, fixes ocaml/dune#7610) - Display cache location in Dune log (ocaml/dune#8974, @nojb) - Re-run actions whenever `(expand_aliases_in_sandbox)` changes (ocaml/dune#8990, @rgrinberg) - Rules that only use internal dune actions (`write-file`, `echo`, etc.) can now be sandboxed. (ocaml/dune#9041, fixes ocaml/dune#8854, @rgrinberg) - Do not re-run rules when their location changes (ocaml/dune#9052, @rgrinberg) - Correctly ignore `bigarray` on recent version of OCaml (ocaml/dune#9076, @rgrinberg) - Add `test_` prefix to default test name in `dune init project` (ocaml/dune#9257, fixes ocaml/dune#9131, @9sako6) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - [coq rules] Be more tolerant when coqc --print-version / --config don't work properly, and fallback to a reasonable default. This fixes problems when building Coq projects with `(stdlib no)` and likely other cases. (ocaml/dune#8966, fix ocaml/dune#8958, @Alizter, reported by Lasse Blaauwbroek) - Dune will now run at a lower framerate of 15 fps rather than 60 when `INSIDE_EMACS`. (ocaml/dune#8812, @Alizter) - dune-build-info: when `version=""` is found in a `META` file, we now return `None` as a version string (ocaml/dune#9177, @emillon) - Dune can now be built and installed on Haiku (ocaml/dune#8795, fix ocaml/dune#8551, @Alizter) - Mark installed directories in `dune-package` files. This fixes `(package)` dependencies against packages that contain such directories. (ocaml/dune#8953, fixes ocaml/dune#8915, @emillon)

emillon · 2023-11-30T15:59:57Z

src/dune_rules/cram/cram_stanza.ml

+     and+ runtest_alias =
+       field_o
+         "runtest_alias"
+         (Dune_lang.Syntax.since Stanza.syntax (3, 11) >>> located bool)


this should be 3.12 since it got added in 3.12.0

rgrinberg force-pushed the ps/rr/feature_cram___allow_overriding_default_alias branch from 728a7e6 to 59b58ef Compare October 9, 2023 01:10

gridbugs reviewed Oct 10, 2023

View reviewed changes

rgrinberg force-pushed the ps/rr/feature_cram___allow_overriding_default_alias branch from 59b58ef to 2271af6 Compare October 12, 2023 22:00

rgrinberg added this to the 3.11 milestone Oct 13, 2023

gridbugs reviewed Oct 13, 2023

View reviewed changes

doc/changes/8887.md Outdated Show resolved Hide resolved

rgrinberg force-pushed the ps/rr/feature_cram___allow_overriding_default_alias branch from 2271af6 to b5a8395 Compare October 13, 2023 04:00

rgrinberg requested a review from christinerose as a code owner October 13, 2023 04:00

gridbugs reviewed Oct 13, 2023

View reviewed changes

rgrinberg force-pushed the ps/rr/feature_cram___allow_overriding_default_alias branch from b5a8395 to 5a11a66 Compare October 13, 2023 04:17

gridbugs approved these changes Oct 13, 2023

View reviewed changes

rgrinberg merged commit b0ea7de into main Oct 13, 2023

rgrinberg deleted the ps/rr/feature_cram___allow_overriding_default_alias branch October 13, 2023 04:28

emillon mentioned this pull request Nov 28, 2023

[new release] dune (15 packages) (3.12.0) ocaml/opam-repository#24862

Closed

emillon reviewed Nov 30, 2023

View reviewed changes

	Now we try setting the defualt aliases twice. This should be impossible:
	Now we try setting the default aliases twice. This should be impossible:

Conversation

rgrinberg commented Oct 9, 2023

Uh oh!

Alizter commented Oct 9, 2023

Uh oh!

rgrinberg commented Oct 9, 2023

Uh oh!

gridbugs Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

gridbugs commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 10, 2023

Uh oh!

gridbugs commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 10, 2023

Uh oh!

gridbugs commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 10, 2023

Uh oh!

gridbugs commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 10, 2023

Uh oh!

gridbugs commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 10, 2023

Uh oh!

gridbugs commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 10, 2023

Uh oh!

gridbugs commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 10, 2023

Uh oh!

rgrinberg commented Oct 12, 2023

Uh oh!

gridbugs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rgrinberg commented Oct 13, 2023

Uh oh!

gridbugs Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

rgrinberg Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

gridbugs Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

rgrinberg commented Oct 13, 2023

Uh oh!

emillon Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants